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

Enter async mode only when necessary #79

Merged
merged 1 commit into from
Jul 3, 2015

Conversation

jscheid
Copy link
Contributor

@jscheid jscheid commented Jun 14, 2015

Hi,

Ever since babel-loader introduced caching we can't use it anymore together with enhanced-require as that doesn't support async callbacks in loaders.

This change restores enhanced-require compatibility as long as the cacheDirectory option isn't used.

(It would be possible to implement caching synchronously but I suppose there are performance concerns. We don't need caching when using enhanced-require so I left caching to be async.)

@jscheid
Copy link
Contributor Author

jscheid commented Jun 16, 2015

Whoops, totally missed #60! Sorry about that. I guess @camshaft and I have the same problem and approached it from different angles. I don't really care which of the two PRs gets merged, it looks like both will do the trick so feel free to close this.

@dashed
Copy link
Contributor

dashed commented Jun 16, 2015

This PR seems more simpler than the other one.

@camshaft
Copy link

the other one does support sync caching though. i could go either way as well.

@Couto
Copy link
Member

Couto commented Jun 17, 2015

The problem with the other one, was that the PR had bad timing since I was rewriting a lot of stuff.
I'm not familiar enough with enhanced-require so I do have to ask... Does caching provide a benefit?

@jscheid
Copy link
Contributor Author

jscheid commented Jul 1, 2015

How about merging this one then, seeing that it's pretty straightforward, and leaving the other one for when somebody finds time to review it?

@Couto
Copy link
Member

Couto commented Jul 1, 2015

@jscheid I think that's a good point, and sorry for taking so long on this matter.
I've merged the PR with the develop branch, can you give a try on some project? If you find no errors with it, I'll make a release with this PR

@jscheid
Copy link
Contributor Author

jscheid commented Jul 1, 2015

@Couto thanks, will do -- switching over from our fork to your develop now. I'll let you know how it goes.

@jscheid
Copy link
Contributor Author

jscheid commented Jul 1, 2015

@Couto seems to work fine!

@Couto Couto merged commit c02e983 into babel:master Jul 3, 2015
@Couto
Copy link
Member

Couto commented Jul 3, 2015

I've merged this. You can find it on npm with version 5.3.0
Thanks everyone. If you find any error, please open an issue :)

I'm not familiar with enhaced-required, if caching is something that benefits its process, please tell me, so that I can open an issue to rework the first PR.

@danez danez mentioned this pull request Nov 18, 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.

4 participants