Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(config): better resolving of components on windows #1128

Merged
merged 1 commit into from
Sep 7, 2018
Merged

Fix(config): better resolving of components on windows #1128

merged 1 commit into from
Sep 7, 2018

Conversation

elevatebart
Copy link
Collaborator

No description provided.

@sapegin
Copy link
Member

sapegin commented Sep 4, 2018

Thanks for the PR! Could you please explain your solution?

@elevatebart
Copy link
Collaborator Author

Absolutely,

The diagnosis is simple: glob.sync("src/@{components,Components}/**/*.js") returns each component file twice on windows since it is case insensitive. It is probably a bug on glob.
In order not to be impacted by this bug or limitation, I make the matching case insensitive. Case is actually never matched on windows anyway.
To avoid disturbing other OSs, I only ignore it on windows.

To avoid having to write the fix twice, I changed the code a little for it to be a little more DRY.

I hope I did not mess too much with the philosophy.
I am open to suggestions.

@sapegin
Copy link
Member

sapegin commented Sep 5, 2018

nocase Perform a case-insensitive match. Note: on case-insensitive filesystems, non-magic patterns will match by default, since stat and readdir will not raise errors.

If I understand this correctly, it's case-insensitive on Windows in any case, so your fix is likely relying on some filtering they do when this flag is enabled. Probably we should report it to glob as well.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments, otherwise looks good!

loaders/utils/getComponentFiles.js Outdated Show resolved Hide resolved
loaders/utils/getComponentFiles.js Outdated Show resolved Hide resolved
loaders/utils/getComponentFiles.js Outdated Show resolved Hide resolved
loaders/utils/getComponentFiles.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Sep 5, 2018

Codecov Report

Merging #1128 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
loaders/utils/getComponentFiles.js 100% <100%> (ø) ⬆️

@elevatebart
Copy link
Collaborator Author

@sapegin Thank you for the feedback. I think I will definitely send a Request to glob to understand better this behaviour. It seems shady.
I took your comments into account and pushed a new version hope you like it.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one last thing ;-)

loaders/utils/getComponentFiles.js Outdated Show resolved Hide resolved
@elevatebart
Copy link
Collaborator Author

Done ;)

@sapegin sapegin merged commit a7a6cf4 into styleguidist:master Sep 7, 2018
@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 7.3.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@elevatebart elevatebart deleted the fix-windowsdouble branch September 9, 2018 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants