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

feat(root): add glob support #78

Merged
merged 2 commits into from Aug 27, 2016
Merged

feat(root): add glob support #78

merged 2 commits into from Aug 27, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2016

I added support for glob. It was a little tricky to integrate that into the current architecture without having the glob running on every import.
For this reason I used a function with memoization.

Now we can do stuff like this:

{
  "plugins": [
    "transform-object-rest-spread",
      ["module-resolver", {
        "root": ["./src/**/shared"]
      }]
    ]
}

and all shared folders are considered roots.
This is useful in architectures like this.

Let me know what you think!

@codecov-io
Copy link

codecov-io commented Aug 26, 2016

Current coverage is 98.43% (diff: 100%)

Merging #78 into master will increase coverage by 0.16%

Diff Coverage File Path
•••••••••• 100% src/index.js

Powered by Codecov. Last update 922857d...e80bdfc

@tleunen
Copy link
Owner

tleunen commented Aug 26, 2016

Thanks @MoeSattler, this is great.
For the memoization, we can also compute things only once per file when using pre() (See what I did in that PR: https://github.com/tleunen/babel-plugin-module-resolver/pull/71/files#diff-1fdf421c05c1140f6d71444ea2b27638R93)

We could create the glob at that time so it can be reused for every import/require in the file.

Also, I'd like your thought on that, but if there're multiple files with the same name in multiple /components, should the plugin warn there's a conflict? Like ./src/xxx/components/Comp1.js and ./src/yyyyyy/components/Comp1.js

@ghost
Copy link
Author

ghost commented Aug 26, 2016

@tleunen Do we have access to the state or the root object in pre()?
Also, wouldn't it make more sense to resolve the glob paths once per execution, not once per file (glob resolution is SLOW!)

@tleunen
Copy link
Owner

tleunen commented Aug 26, 2016

Don't remember what we have access to :/
There's probably a hook to do it once per execution (or outside of the exported object?), but I currently don't know it.

@ghost
Copy link
Author

ghost commented Aug 26, 2016

@tleunen for now I settled with manipulateOptions, where I intercept the users root options and replace the globs with actual paths. What do you think?

Also, I'd like your thought on that, but if there're multiple files with the same name in multiple /components, should the plugin warn there's a conflict? Like ./src/xxx/components/Comp1.js and ./src/yyyyyy/components/Comp1.js

Good question.
node usually looks in the parents folder for node_modules. If it isn't there it looks into the parent's parent and so on.
Though, imitating this is out of scope. I think as long as we cannot resolve this, a warning would be fair.

@ghost
Copy link
Author

ghost commented Aug 26, 2016

The glob operation seems to take too long on CI

btw, you are using arrow functions for tests, which is discouraged:
https://mochajs.org/#arrow-functions
I changed it to function so I can use this.timeout(x)

@@ -1,25 +1,29 @@
/* eslint-env mocha */
/* eslint-disable prefer-arrow-callback */
/* eslint-disable func-names */
Copy link
Owner

Choose a reason for hiding this comment

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

Could you just name the anonym fn with "test" instead? You should be able to remove these 2 eslint rules after that

Copy link
Author

@ghost ghost Aug 27, 2016

Choose a reason for hiding this comment

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

It still would warn me prefer-arrow-callback.

Also I'd need to give the upper scope functions (root and alias) different names to not trigger the no-shadow rule.

@tleunen
Copy link
Owner

tleunen commented Aug 26, 2016

Thanks for the great contribution @MoeSattler.
In term of performance, how bad is it when having a glob resolution? Is it possible the bad performance is caused by the test? Like ./**/components will look in every directory of the project (even inside node_modules), but if you change for ./test/**/components, it should be better. And also show a better example of what users would probably do.

@ghost
Copy link
Author

ghost commented Aug 27, 2016

You were of course right. Giving a proper test case removed the speed issue entirely.

Still, while non-glob paths are not affected (people not using globs won't be able to tell the difference), glob lookups take some more time.
But with reasonable globs (and by reasonable I mean not crawling the node_modules folder as I did 😆 ) it should be okay.
In a complex, deeply and widely nested project I worked on I had a glob that took up to 500ms. Though this isn't THAT awful considering it's during build time.

@tleunen
Copy link
Owner

tleunen commented Aug 27, 2016

Thanks for the contribution @MoeSattler.
I'm preparing an update in the tests, and also another fix and then I'll release a new version with your feature :)

@tleunen tleunen merged commit 1f6245b into tleunen:master Aug 27, 2016
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.

2 participants