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

fix(dynamic-import-vars): allow ./${var}.suffix.js #834

Conversation

lennyburdette
Copy link
Contributor

@lennyburdette lennyburdette commented Mar 12, 2021

Rollup Plugin Name: dynamic-import-vars

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Allow dynamically importing files in the same directory with an extra suffix before the extension.

My motivating example is trying to import a library generated with Stencil. It generates a dynamic import that looks like this:

return import(
    /* webpackInclude: /\.entry\.js$/ */
    /* webpackExclude: /\.system\.entry\.js$/ */
    /* webpackMode: "lazy" */
    `./${bundleId}.entry.js${ ''}`)

My interpretation of the README suggests this should be possible. I changed the assertion for "Imports to your own directory must specify a filename pattern" to a stricter regex. I'm happy to change the implementation if regexes aren't desired.

Thanks!

Allow dynamically importing files in the same directory with an extra suffix before the extension.
@LarsDenBakker
Copy link
Contributor

This effectively disables the check. I think it's better to add an option which allows these kinds of imports, although they're quite dangerous because the glob will match your current file as well.

@lennyburdette
Copy link
Contributor Author

My assumption was that if ./mod-${id}.js was allowed, then ./${id}.mod.js should also be allowed — both technically allow matching the current file if it's named "mod-current.js" or "current.mod.js".

But! For my specific use-case I discovered that Stencil provides a tree-shakable, non-lazy-loading build, so I'm unblocked. Feel free to close or amend this however you like. Thanks for the review!

@LarsDenBakker
Copy link
Contributor

Ah I see, do we allow ./mod-${id}.js currently? I guess that makes sense then.

@lennyburdette
Copy link
Contributor Author

yep!

// not allowed
import(`./${foo}.js`);
// allowed
import(`./module-${foo}.js`);

what's tripping me up is that the Stencil file output is is component-name.entry.js — using a period makes the current startsWith think that a file extension is next, not a module name suffix.

@shellscape
Copy link
Collaborator

@LarsDenBakker @danielgindi @tjenkinson gentle ping for review

@shellscape
Copy link
Collaborator

@LarsDenBakker @danielgindi @tjenkinson another gentle ping. I'm inclined to move this one forward.

@danielgindi
Copy link
Contributor

Looks good to me and makes sense.

Copy link
Contributor

@LarsDenBakker LarsDenBakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well. sorry haven't been spending a lot of time on open source lately

@shellscape shellscape merged commit f48886f into rollup:master Jul 15, 2021
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
@lennyburdette
Copy link
Contributor Author

Thank you!

@lennyburdette lennyburdette deleted the dynamic-import-vars-dot-prefixed-suffixes branch July 15, 2021 21:13
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape pushed a commit that referenced this pull request Jul 15, 2021
Allow dynamically importing files in the same directory with an extra suffix before the extension.
shellscape added a commit that referenced this pull request Jul 15, 2021
* fix(dynamic-import-vars): allow ./${var}.suffix.js (#834)

Allow dynamically importing files in the same directory with an extra suffix before the extension.

* enforce `prettier` is happy on CI

* run prettier on more things in precommit

and do not pass `--single-quote` to use the value in the config

* run `pnpm run prettier`

* remove redundant `--plugin` option

because it will be loaded automatically given it's in the config

* chore: add changelogs to docs lint

Co-authored-by: Lenny Burdette <lennyburdette@gmail.com>
Co-authored-by: Andrew Powell <shellscape@users.noreply.github.com>
Co-authored-by: shellscape <andrew@shellscape.org>
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