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

Make transform stateless #71

Merged
merged 6 commits into from Jun 7, 2016

Conversation

Projects
None yet
3 participants
@Rich-Harris
Copy link
Contributor

commented Jun 7, 2016

This follows up on @tivac's suggestion in rollup/rollup#658 – rather than tracking which CommonJS features are used and injecting them haphazardly using the intro hook, modules import commonjsHelpers. This is cleaner, and will allow the results of the transformation to be cached.

There is a wrinkle however: the synthetic ID used for the helpers goes through the same resolution process as any other ID. Since in this case that frequently means rollup-plugin-node-resolve, we end up passing trying to resolve whatever ID we use with the Node resolution algorithm, which will obviously fail.

So I'm proposing we adopt the following convention: plugins that generate synthetic helper modules, like this one, should use the null character (\0) in the ID. Plugins that implement resolveId should ignore IDs with the null character. (The reason I'm suggesting NUL is that it's invalid in filenames on both *nix and Windows.)

I've already taken the liberty of updating createFilter and node-resolve.

Any thoughts @rollup/collaborators?

Apologies for the bad git discipline, I managed to roll some extra stuff in here 😕

@@ -37,13 +37,20 @@ function getCandidates ( resolved, extensions ) {
);
}

const HELPERS_ID = '\0commonjsHelpers';
const HELPERS_NAME = '__commonjsHelpers';

This comment has been minimized.

Copy link
@eventualbuddha

eventualbuddha Jun 7, 2016

Member

I realize this is no worse than it was, but this should be generated so as not to conflict with anything in the file. Could do a simple indexOf with an incrementing suffix rather than trying to be clever with scopes.

This comment has been minimized.

Copy link
@Rich-Harris

Rich-Harris Jun 7, 2016

Author Contributor

good point – fixed

@Rich-Harris Rich-Harris merged commit 6e8b598 into master Jun 7, 2016

2 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Rich-Harris Rich-Harris deleted the stateless branch Jun 7, 2016

@Victorystick

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

Neat! Prepending a module name with NUL is a decent hack. I'll update TypeScript accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.