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: add dynamicImportFunction option #2723

Merged

Conversation

keithamus
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

N/A

Description

This option allows users of esm format to rename the import() function, which right now has mixed support in browsers. By allowing this function to be renamed we can have polyfills (which cannot use the reserved import keyword but could go by a different name like importModule).

Right now if we chose to use esm format, and use dynamic imports, then the output source text uses the import() function - this fails in browsers that don't support import(). By adding this option, and renaming import() to something else, we have a chance at using dynamic imports before all browsrs support them.

This option allows users of `es` format to rename the `import()`
function, which right now has mixed support in browsers. By allowing
this function to be renamed we can have polyfills (which cannot use the
reserved `import` keyword but could go by a different name like
`importModule`).
@keithamus keithamus force-pushed the feat-add-dynamicimportfunction-option branch from ee6b1f2 to 9a17e68 Compare February 26, 2019 11:55
@lukastaegert
Copy link
Member

Thanks, I like the feature, implementation looks also fine (though there is still an option list in the tests that needs to be adjusted I assume). Some thoughts:

  • It would be nice to get a warning that this option is ignored for non-esm targets unless the format is esm
  • the option should be added to the documentation (big list of options + other option lists). I assume doing the actual polyfill is not something that can be jotted down in a few lines or maybe there is a link to one or some documentation that can be added?
  • the option should be added to the CLI options list (help.md)

@keithamus
Copy link
Contributor Author

keithamus commented Mar 1, 2019

Okay @lukastaegert I believe this addresses your points. Thanks for fixing the tests 😄

@lukastaegert
Copy link
Member

Nice one, thanks a lot, especially for the link to the polyfill!

@lukastaegert lukastaegert merged commit 91ce981 into rollup:master Mar 1, 2019
@keithamus keithamus deleted the feat-add-dynamicimportfunction-option branch March 1, 2019 10:24
@tomalec
Copy link

tomalec commented Nov 26, 2019

Why is it limited to es format only?

I have a use case for thins for iife.
The code is meant to be run by Node and browser.

if(typeof fetch === 'undefined'){ // in node shim JSON modules with imported require
    const createRequire = await import('module').then(m=>m.createRequire);
    const require = createRequire(import.meta.url);
    currentVersion = require('../../package.json').version;
 } else { // in browser shim with fetch
    currentVersion = await fetch('../../package.json')
        .then(response => response.json())
        .then(json => json.version);
}

On Node it runs flawlessly, in modern browsers as well, but old Edge is throwing a Syntax Error for import(.

I would like to use dynamicImportFunction: "somethingElseForEdge" to let Edge parse the code.

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.

None yet

3 participants