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 list of known js source extensions to config #3828

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dominikg
Copy link
Contributor

Description

implementation of #3219 with config option at runtime as requested in #3219 (comment)

Separate PR to be able to compare the two and avoid having to rebase/force-push

I don't really like how the array is passed through several function calls to reach the util, but without some structural changes this is the least invasive i could manage.

Additional context


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.

Comment on lines +291 to +293
if (!config.knownJsSrcExtensions) {
config.knownJsSrcExtensions = [...DEFAULT_KNOWN_JS_SRC_EXTENSIONS]
}
Copy link
Member

Choose a reason for hiding this comment

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

We can wait until we discuss this with Evan, but I think it is better if this works like assetsInclude. So the user will give us extra extensions and they are added to the ones supported by default by vite.

@@ -416,6 +447,7 @@ export async function resolveConfig(
assetsInclude(file: string) {
return DEFAULT_ASSETS_RE.test(file) || assetsFilter(file)
},
knownJsSrcExtensions: resolvedKnownJsSrcExtensions,
Copy link
Member

Choose a reason for hiding this comment

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

We discussed a bit about this in Discord. IMO we could also use the same scheme utilized by assetsInclude here. In the ResolvedConfig, we will have a resolved.isJsRequest(path) function that plugins can then use. This is something else to discuss with Evan, if we go here, we may want to have all these helpers in resolve.utils.isJsRequest, but I am not sure if it is really needed and the flat structure is enough here. @antfu what do you think?

@Shinigami92 Shinigami92 requested a review from antfu June 17, 2021 07:35
@patak-dev
Copy link
Member

Commenting for reference, as this was already discussed in Discord. We may be able to avoid the need for defining these known imports altogether, so for the moment is better to hold on with exposing a new config option here. We can continue to accept new known types if that helps the ecosystem in the meanwhile. @dominikg I'll make this one a draft for the time being.

@benmccann
Copy link
Collaborator

We may be able to avoid the need for defining these known imports altogether, so for the moment is better to hold on with exposing a new config option here.

@patak-dev do you happen to remember how it was that we might be able to avoid defining these known imports? Is there any update on that or whether we should move forward with this PR? I see you referred to this PR positively in a recent comment, so I'm wondering if there might be a path forward for it if it were rebased?

@espipj
Copy link

espipj commented May 9, 2023

Hello! I am finding hard to make vite to pre-process .feature files (cucumber) which seems related to this PR and this issue #9963
is there any timeline for these changes to be merged into main? is there any way I can help?

@bluwy
Copy link
Member

bluwy commented Oct 23, 2024

I believe in the last meeting we had decided to close this in favour of #9981. We don't completely remove isJSRequest yet due to concerns that Sec-Fetch-Dest is still relatively new, but eventually when Vite bumps its dev browser support that will already support it, we can drop isJSRequest, or start warning if we have to rely on it.

@dominikg
Copy link
Contributor Author

not sure i like the way plugins have to use configureServer to set that header manually, thats a lot of repeated code for a common need. most frameworks building on vite have a custom extension for their files so we should provide a better util than #9981 (comment) for them to set this up

@bluwy
Copy link
Member

bluwy commented Oct 23, 2024

Isn't Sec-Fetch-Dest automatically set by the browser? On the latest browsers that sends that, it should work by default with that PR. On browsers that do not support that and isn't in the hardcoded list now, they can workaround with the link you showed which I think is fine as it's only a stop gap.

@dominikg
Copy link
Contributor Author

browsers are never going to set that header themselves if they see an import to .vue or .svelte. so that middleware would always be needed for any extension that is not natively considered script by browsers.

@bluwy
Copy link
Member

bluwy commented Oct 23, 2024

Why not? It works for me:
image

It also works for components imported from a Svelte component compiled to JS:
image

@dominikg
Copy link
Contributor Author

so they add it based on the fact that the url was given to it either in the src value of a script tag or imported from such a script?
Is this part of the actual spec or just current behavior? Also what about non-browsers, do we expect a curl call to a .svelte file to set that header and would it serve it raw otherwise?

@bluwy
Copy link
Member

bluwy commented Oct 24, 2024

Yes browsers will add that information. The spec doesn't mention requiring to add that information, but if browsers already did, I don't see them not doing it anymore without breaking the web. For non-browsers when we drop isJSRequest, then it'll always be served raw unless you add that header yourself.

@dominikg
Copy link
Contributor Author

i believe that would be an unneccessary regression, an internal vite:tag-js-requests plugin that reads a custom js extensions config and injects that header before it reaches the isJS logic would help with this.

Also is this purely a dev concern or something that also needs to be available at buildtime?

@bluwy
Copy link
Member

bluwy commented Oct 24, 2024

It's dev only. I personally don't think it'll be a big regression in practice, unless you're fetching the resource directly from curl or testing, but those things aren't common for most users who develop on Vite-based projects.

We could discuss this in the next meeting again, but either ways I don't think this discussion is blocking from merging the other PR as it has other benefits too.

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
Status: Rejected
Development

Successfully merging this pull request may close these issues.

6 participants