Skip to content

Rewrite all require('babel-runtime/**) statements to absolute paths #400

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

Merged
merged 1 commit into from
Mar 19, 2016

Conversation

jamestalmage
Copy link
Contributor

I think messing with the module path is a bad idea, it seems like an easy way to create hard to reproduce bugs.

Also, if a user installs their own version of babel-runtime, we have no guarantee it is compatible with the one needed by our tests. By rewriting to absolute paths, we can completely bypass whatever version the user installs locally.

This PR adds a babel-plugin which rewrites all babel-runtime requires to absolute paths.

@ariporad
Copy link
Contributor

ariporad commented Jan 2, 2016

@jamestalmage: I think that it would be a much simpler if we just modified the babel-runtime plugin. This seems like something that could easily fall apart.

@jamestalmage
Copy link
Contributor Author

I think that it would be a much simpler if we just modified the babel-runtime plugin.

babel-transform-runtime is not under our control, and I doubt the Babel team would ever accept a change that used absolute paths, the current behavior is intentional on their part.

This seems like something that could easily fall apart.

You will need to defend that statement with clear examples of how that might happen.

@sindresorhus
Copy link
Member

I think messing with the module path is a bad idea

👍

@sindresorhus
Copy link
Member

@jamestalmage Anything blocking merging this other than the merge conflict?

@jamestalmage
Copy link
Contributor Author

Only if there is a valid reason not to perform this transform. I don't see it causing any problems, and I think it solves other (i.e. they can require('bluebird') in npm@2 even if they don't have bluebird installed).

I will look into rebasing this over the weekend.

@novemberborn
Copy link
Member

👍

@sindresorhus
Copy link
Member

@jamestalmage Any chance you could rebase this? This PR have been open forever and would be nice to get it resolved :)

@sindresorhus sindresorhus changed the title [Seeking Feedback]: Rewrite all require('babel-runtime/**) statements to absolute paths. Rewrite all require('babel-runtime/**) statements to absolute paths Mar 15, 2016
@novemberborn
Copy link
Member

Sounds like a reasonable idea. We could then wrap babel-runtime to reduce installed package size too.

@jamestalmage
Copy link
Contributor Author

rebased

@sindresorhus
Copy link
Member

LGTM

@sindresorhus
Copy link
Member

We could then wrap babel-runtime to reduce installed package size too.

@novemberborn Could you open an issue about this?

jamestalmage added a commit that referenced this pull request Mar 19, 2016
Rewrite all `require('babel-runtime/**)` statements to absolute paths
@jamestalmage jamestalmage merged commit e4ef5f2 into avajs:master Mar 19, 2016
@jamestalmage jamestalmage deleted the rewrite-runtime branch March 19, 2016 21:18
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