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: configurable known js source extensions #3219

Closed

Conversation

dominikg
Copy link
Contributor

Description

Introduce a config option to allow plugins/users to contribute to the logic of vites isJSRequest util

see discussion here #2829

As this option is primarily aimed at plugin authors i didn't add documentation to the user docs. Can be done if needed/wanted

This PR is also a first step in a series that would have to be taken to allow current and future SFC style plugins to configure vite without relying on the help of vite internals. The discussion linked above highlights other areas that are also in need of configurability

Additional context

The current implementation of isJSRequest returns true for foo.txt?bar=.js because it ended with .js and cleanUrl was called after the regex check. If that semantic was on purpose it could be added back by constructing a similar regex out of the Set


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

// update known js src extensions
config.knownJsSrcExtensions?.forEach((ext) =>
KNOWN_JS_SRC_EXTENSIONS.add(ext.startsWith('.') ? ext : `.${ext}`)
)
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't modify KNOW_JS_SRC_EXTENTIONS here, but create a new knownJsSrcExtensions entry in ResolvedConfig that have both the default plus the added by plugins. You should be able to run Vite build programmatically with different plugins without interfering with each other (I have doubts about the cache, but for the rest, it should be independent). This would imply that we have to pass this to isJsRequest, I hope there isn't that much wiring to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that wiring was the reason i resorted to this. but you're right that modifying that isn't stable esp. if config changes. Should it be a string array in the ResolvedConfig or a Set or should ResolvedConfig have a isJSRequest function (like assetsInclude already is)

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 30, 2021
Comment on lines +25 to +27
'.vue',
'.marko',
'.svelte'
Copy link
Member

Choose a reason for hiding this comment

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

When we published this feature we could contact the vue, marko and svelte plugin owners and ask for that they add their extensions via their plugins
Then later on we should remove these framework extensions from Vite core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ultimately that is the goal. For now i kept them because it requires plugin support and users updating said plugins. Maybe they should be marked as deprecated and removed with the next major only.
As this hasn't been widely used and if we coordinate it correctly with the plugins removal can happen sooner 🤔

Copy link
Member

Choose a reason for hiding this comment

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

We are going to release a minor version in around 10 days, we may be able to do it there. I think maybe we could deprecate them in v2.3 and wait til v2.4 to remove them.

Copy link
Member

Choose a reason for hiding this comment

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

@patak-js let the plugin-owners and the eco-system catch up and remove them in e.g. vite 2.6+

@dominikg
Copy link
Contributor Author

updated implementation to pass config.knownJsSrcExtensions into isJSRequest in #3828

@dominikg dominikg closed this Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants