cramforce / splittable

Module bundler with support for code splitting, ES6 & CommonJS modules.
Apache License 2.0
945 stars 34 forks source link

Add import() support #32

Open chicoxyzzy opened 7 years ago

chicoxyzzy commented 7 years ago

It should replace System.import method https://github.com/tc39/proposal-dynamic-import

The proposal can possibly move to stage 3 next week

cramforce commented 7 years ago

Happy to take a PR :)

On Nov 27, 2016 9:31 AM, "Sergey Rubanov" notifications@github.com wrote:

It should replace System.import method https://github.com/tc39/proposal-dynamic-import

The proposal can possibly move to stage 3 next week https://github.com/tc39/agendas/blob/master/2016/11.md

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cramforce/splittable/issues/32, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT1ZLtQkeLkd2OpkPxSR82EQzk2vXks5rCb5TgaJpZM4K9M8N .

chicoxyzzy commented 7 years ago

It is on stage 3 now

chicoxyzzy commented 7 years ago

import is reserved words so dynamic imports can't be polyfilled. They should be transpiled. So we probably need Babel plugin first

chicoxyzzy commented 7 years ago

There is https://github.com/babel/babel/tree/master/packages/babel-plugin-syntax-dynamic-import so I suppose we can use custom plugin to transpile it to current System.import. @cramforce are you ok with that?

cramforce commented 7 years ago

Definitely would need to be polyfilled using rewriting of files. The main thing is resolving the module identifier to a bundle URL.

On Thu, Dec 1, 2016 at 2:57 PM, Sergey Rubanov notifications@github.com wrote:

import is reserved words so dynamic imports can't be polyfilled

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cramforce/splittable/issues/32#issuecomment-264321826, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT7VjvmF7HuVhFrgljyvQRtCBFdJqks5rD1DrgaJpZM4K9M8N .

cramforce commented 7 years ago

Just saw the second comment. My email was delayed.

Yeah, you can insert that path in here: https://github.com/cramforce/splittable/blob/master/splittable.js#L223

And then look for System.import in the evil regex below and that might work!

chicoxyzzy commented 7 years ago

I just created babel plugin which replaces import() with System.import(). I think it could be added to line you mentioned but tests look complicated. I didn't figure out how to add tests for import()

cramforce commented 7 years ago

Cool. I can take it from there. Does the plugin do nothing except s/import/System.import. One very useful feature would be a hook to control rewriting of the module argument (Going from module identifier to URL).

chicoxyzzy commented 7 years ago

Yes, it replaces dynamic imports with System.imports but not just s/import/System.import because we should replace only import calls, not static imports.

One very useful feature would be a hook to control rewriting of the module argument (Going from module identifier to URL).

Not sure if this logic should be in babel plugin. System.import needs this too, right?

cramforce commented 7 years ago

It cannot be correctly done at runtime. The nice thing about import is that it resolves a relative module name. A regular function call cannot know what '../foo' means, because it doesn't know about filenames anymore.

chicoxyzzy commented 7 years ago

That's true. Can you explain what changes do you want from transformer?

cramforce commented 7 years ago

@chicoxyzzy It would either be configurable or something predictable. E.g. if the resulting path was always a relative path from the project root instead of the current file, then everything else could be done at runtime.

chicoxyzzy commented 7 years ago

ok I'll try to add an option for import specifier to be exactly StringLiteral and a relative path.