Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) #440

dotcs · 2017-05-12T13:48:52Z

This PR fixes the getExampleFilename method and re-implements the logic such that the filename can be case-insensitive as requested here: #423 (comment).

sapegin

Thanks, this is very cool feature!

sapegin · 2017-05-12T14:41:48Z

scripts/schemas/config.js

+			// Search is case-insensitive, so `README.md` or `BUTTON.md` is also
+			// valid.
+			const readmeIndex = lowerFiles.indexOf('readme.md');
+			const componentIndex = lowerFiles.indexOf(`${componentName.toLowerCase()}.md`);


I’d move this line after the first condition; and you don't need an else because you have return.

Done. I think we still need else branch because it can be that neither a readme.md nor a mycomponent.md file is available. In this case readmeIndex and componentIndex have both the value -1 and both conditions are wrong. In this case false is returned as in the previous implementation. Please have a look in the updated code.

sapegin · 2017-05-12T14:47:42Z

scripts/schemas/config.js

-				if (fs.existsSync(file)) {
-					return file;
-				}
+			const files = fs.readdirSync(dirname);


Should we cache it somehow? Imagine a project with all components in a single folder.

I've had a look, but I'm not sure where caching could be done. getExampleFilename is called in loaders/props-loader.js and processComponent.js. The first one is already cached via webpack as it's a cachable loader (please correct me if I'm wrong). The second one is called to process a component in case it has changed, so I think in this case we should update anyway. Where in the application logic would you see the caching layer?

I mean to cache readdirSync because it will be called for each component possible with the same results if all of them are in the same folder.

dotcs · 2017-05-12T15:21:45Z

@sapegin Thanks for the feedback. I'll have a look hopefully somewhen later this weekend. I'll see if I can implement cache and see what I can do about failing tests (somehow they worked locally ... ??). I'll come back to you when I'm done.

…es named after component)

…uidist into case-insensitive-readme-matching

codecov-io · 2017-05-27T10:23:07Z

Codecov Report

Merging #440 into master will increase coverage by 0.05%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
+ Coverage   94.32%   94.38%   +0.05%     
==========================================
  Files          82       82              
  Lines        1146     1158      +12     
  Branches      254      258       +4     
==========================================
+ Hits         1081     1093      +12     
  Misses         61       61              
  Partials        4        4

Impacted Files	Coverage Δ
src/rsg-components/Playground/Playground.js	`84.37% <ø> (ø)`	⬆️
src/rsg-components/Preview/Preview.js	`50% <0%> (ø)`	⬆️
loaders/utils/getProps.js	`100% <100%> (ø)`	⬆️
loaders/utils/getComponentFiles.js	`100% <100%> (ø)`	⬆️
loaders/utils/client/evalInContext.js	`100% <100%> (ø)`	⬆️
loaders/styleguide-loader.js	`100% <100%> (ø)`	⬆️
loaders/props-loader.js	`79.31% <100%> (+0.73%)`	⬆️
scripts/utils/sanitizeConfig.js	`100% <100%> (ø)`	⬆️
src/utils/utils.js	`96.96% <100%> (ø)`	⬆️
scripts/make-webpack-config.js	`100% <100%> (ø)`	⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97d233b...99d7e61. Read the comment docs.

dotcs · 2017-05-27T10:23:59Z

@sapegin Sorry for the long delay. I've been quite busy within the last two weeks. I've implemented a cache layer into the getExampleFilename implementation. Can you have a look if this fits what you have thought of?

The idea goes like this:
The props-loader is marked as cachable, so webpack will only call the real implementation if the underlying file system has changed somehow. In this case we have to invalidate the cache. But for all calls of getExampleFilename within one iteration after files have changed the cache can be used to reduce file io reads.

What this PR misses is documentation of the cache as the second param of the getExampleFilename implementation. I leave this up to you if you think it should be documented.

Side note: I had to fix some lint issues (see commit dcf17a9) because code from master was invalid...?!

sapegin · 2017-05-28T10:54:46Z

Side note: I had to fix some lint issues (see commit dcf17a9) because code from master was invalid...?!

Looks like you need to npm install after merging master to your branch.

sapegin · 2017-05-28T11:05:00Z

scripts/schemas/config.js

 	getExampleFilename: {
 		type: 'function',
-		default: componentPath => {
+		default: (componentPath, cache) => {


Could you please explain benefits of your implementation over _.memoize?

I’d make a cached case-insensitive version of fs.existsSync and pass it to to getExampleFilename. Passing just an object doesn’t look useful and will be hard to explain in the docs.

Close #423, #460, #440, #482.

@SaraVieira

## New features ### Copy pathline to clipboard button ![](http://wow.sapegin.me/3t2u3S0P0L2T/Image%202017-06-28%20at%204.17.16%20PM.png) (#485, #471 by @SaraVieira) ### Easier way to override style guide React components New config option `styleguideComponents` to override React components used to render a style guide. ```javascript module.exports = { styleguideComponents: { Logo: path.join(__dirname, 'styleguide/components/Logo'), StyleGuideRenderer: path.join(__dirname, 'styleguide/components/StyleGuide'), }, }; ``` (#504) ### Customizable logging New `logger` option: ```javascript module.exports = { logger: { // One of: info, debug, warn // Suppress messages info: () => {}, // Override display function warn: message => console.warn(`NOOOOOO: ${message}`), }, }; ``` (#472, #510) ## Bug fixes * Allow `devServer.watchConfig` to be configured, this also means Vagrant is now supported (#515, #516 by @esphen) * Default `getExampleFilename` should be case-insensitive (#423, #460, #440, #482) * Allow dynamic JSS styles * Isolate search placeholder styles (#509, #491 by @n1313)

dotcs added 2 commits May 12, 2017 15:47

Fix: Rewrite getExampleFilename logic to make it case insensitive (#423)

eb62c98

Merge branch 'master' into case-insensitive-readme-matching

7d91d5c

sapegin reviewed May 12, 2017

View reviewed changes

dotcs changed the title ~~Fix: Rewrite getExampleFilename logic to make it case insensitive (#423)~~ (WIP) Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) May 12, 2017

dotcs added 5 commits May 13, 2017 08:31

Update snapshots

cdd1531

Improve execution order (test for readme first and fallback to md fil…

b9578d8

…es named after component)

Merge branch 'master' of https://github.com/styleguidist/react-styleg…

54f7545

…uidist into case-insensitive-readme-matching

Fix lint issues

dcf17a9

Add caching layer to default getExampleFilename implementation

99d7e61

dotcs changed the title ~~(WIP) Fix: Rewrite getExampleFilename logic to make it case insensitive (#423)~~ Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) May 27, 2017

sapegin reviewed May 28, 2017

View reviewed changes

sapegin mentioned this pull request May 30, 2017

Why 'Readme.md' vs 'README.md'? #460

Closed

stepancar mentioned this pull request May 31, 2017

docs(*): fix styleguide docs for linux alfa-laboratory/arui-feather#52

Merged

sapegin mentioned this pull request Jun 9, 2017

Makes getExampleFilename case insensitive #482

Closed

sapegin added a commit that referenced this pull request Jun 24, 2017

Fix: Default getExampleFilename should be case-insensitive

84e508e

Close #423, #460, #440, #482.

sapegin closed this Jun 24, 2017

This was referenced Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#165

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#166

Open

kopax mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities rollup-umd/react-styleguidist#132

Open

This was referenced Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#175

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#176

Open

kopax mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities rollup-umd/react-styleguidist#142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) #440

Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) #440

Uh oh!

dotcs commented May 12, 2017

Uh oh!

sapegin left a comment

Uh oh!

sapegin May 12, 2017

Uh oh!

dotcs May 14, 2017

Uh oh!

sapegin May 12, 2017

Uh oh!

dotcs May 14, 2017

Uh oh!

sapegin May 14, 2017

Uh oh!

dotcs commented May 12, 2017

Uh oh!

codecov-io commented May 27, 2017 •

edited

Loading

Uh oh!

dotcs commented May 27, 2017 •

edited

Loading

Uh oh!

sapegin commented May 28, 2017

Uh oh!

sapegin May 28, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) #440

Fix: Rewrite getExampleFilename logic to make it case insensitive (#423) #440

Uh oh!

Conversation

dotcs commented May 12, 2017

Uh oh!

sapegin left a comment

Choose a reason for hiding this comment

Uh oh!

sapegin May 12, 2017

Choose a reason for hiding this comment

Uh oh!

dotcs May 14, 2017

Choose a reason for hiding this comment

Uh oh!

sapegin May 12, 2017

Choose a reason for hiding this comment

Uh oh!

dotcs May 14, 2017

Choose a reason for hiding this comment

Uh oh!

sapegin May 14, 2017

Choose a reason for hiding this comment

Uh oh!

dotcs commented May 12, 2017

Uh oh!

codecov-io commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dotcs commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapegin commented May 28, 2017

Uh oh!

sapegin May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented May 27, 2017 •

edited

Loading

dotcs commented May 27, 2017 •

edited

Loading

sapegin May 28, 2017 •

edited

Loading