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: support custom modifiers for glob imports #7209
Conversation
Note: We might want to refrain extending |
@bluwy Thanks for mentioning #7017.
@patak-dev I understand the motivation to delay further import modifiers. The thing is that this PR is a blocker for @thetre97 who is building a Gridsome-like framework on top of vps, and it's also a blocker for the framework I'm currently building on top of vps and Telefunc. Maybe, if it's ok from your perspective, we could merge this is in and keep it undocumented until we figure out #7017.
This is not really something new, vps already uses a custom modifier |
Yeah, I mean making Re merging first, I think we can still implement #7017 first though. Most of the ground work has be done to support the |
Makes sense.
I'll update my PR to also implement this. |
@brillout would you explain the complete use case for both frameworks? What modifiers do they need? Or do they need this to be open-ended for users? |
Imagine a framework // node_modules/some-framework/renderer/_default.page.client.js
export { render }
import ReactDOM from 'react-dom'
import React from 'react'
async function render(pageContext) {
const { Page, pageProps } = pageContext
ReactDOM.hydrate(
<Page {...pageProps} />,
document.getElementById('view')
)
} To take control over // pages/_default.page.client.js (this file lives in user-land)
export { render }
// Override the `_default.page.client.js` provided by `some-framework`:
export const overrideDefaults = true
import ReactDOM from 'react-dom'
import React from 'react'
async function render(pageContext) {
// Here the user takes control over how her pages are rendered/hydrated
} Now the thing is that Node.js needs to know which So vps uses a custom modifier // pages/_default.page.client.js?meta
export const exportNames = ['render', 'overrideDefaults'] That way Node.js can have some meta information about the Now, with the vps changes I'm currently working on, the client-side also needs meta information about // node_modules/vite-plugin-ssr/client/index.js
// Each `.page.client.js` file is loaded only if needed
const pageFiles = import.meta.glob('/**/*.page.client.js')
// Load the meta information of all `.page.client.js` files
const pageFilesMeta = import.meta.globEager('/**/*.page.client.js', { as: 'meta' }) That way the vps client can have meta information about all The Let me know if something's not clear or if you have any question. |
285fc83
to
d8362ca
Compare
Done. This PR now also implements #7017. |
Would it be possible to split this PR in two so we can merge the fix for #7017 during the 2.9 beta? The feat part still needs to be discussed in a team meeting (probably this Friday). If not, I'll send a PR based on your work to implement the migration to |
Will do. Re the feat part. The way I see it is that it's a bug fix more than a feature (the reason I marked it as But maybe I'm missing something here :-). |
Fair enough, but we would still need to discuss this with the team. I personally think that you have a point and reading through https://github.com/tc39/proposal-import-reflection, it looks like the import FooModule from "./foo.wasm" as "wasm-module" We know that user suffixes will only collide with Vite and the ecosystem, but with the |
2a41ae3
to
8c45a5e
Compare
👍 Makes sense. Personally, I've a slight preference for the nicer aesthetics of omiting a guard over the minor reliability increase of having a guard. But I'm fine either way :-). PR is now split:
|
8c45a5e
to
0d7d551
Compare
This PR now contains only the feat. It's rebased and ready to be merged. Sum up of why vps needs this. vps has a // pages/product.page.client.js
export const useClientRouter = true
// Some Vue/React component
export const Page = /* ... */
// This file can be quite large
/* ... */ // pages/product.page.client.js?meta (after transformation)
export const exportNames = ['useClientRouter', 'Page']
// This file is now only one LOC (The Thanks to the // node_modules/vite-plugin-ssr/dist/client/router.js
// The vps client router needs to know all `export` of all `.page.client.js`
const clientPageFilesMeta = import.meta.globEager('/**/*.page.client.js', { as: 'meta' })
console.log(clientPageFilesMeta['/pages/product.page.client.js'].exportNames) // ['useClientRouter', 'Page'] (It would be prohibitive to I'm currenlty working on a vps refactor and the refactor needs this. (And the frameworks being built on top of vps need this refactor.) |
@brillout we discussed with part of the team in the last meeting, and what was proposed was that this should be supported by a plugin at the moment. Maybe as |
vps now uses a custom plugin instead of a glob import. Other than HMR, it works, and I've an idea for making HMR work. Although vps doesn't need this PR anymore, I still believe we should merge it. Let me elaborate. The reason we don't merge this is because we are not sure whether we will keep supporting the Let's imagine we deprecate this syntax and what merging this PR would then mean. If we don't merge this PR, the message will be:
If we do merge this PR, the message will be:
The bottom line: the two messages are not really different.
Precisely, we do not extend the syntax. It's only one syntax. Either we continue to support the syntax or we depreate it, but either way it doesn't really make a difference what the user does with that syntax. That said, merging this PR does mean that more users will have to move to the new syntax. But I don't think we should block users from using custom modifiers because we are not sure about the syntax. It seemed to me that we already settled for the One thing we can do is to mark the If we want to be very conservative, we can even warn the user with something like:
This essentially enables us to deprecate the syntax whenever we want. Either way, supporting the Writing the plugin did consume quite some time and we should spare the Vite ecosystem that effort. Custom modifiers are crucial. Beyond vps now has first-class support for using Vue/React/... only on the server-side (zero/minimal browser-side JavaScript) and the Such modifier will also be needed for React Server Components. (It's a new React 18 technique to write server-side components that never get loaded in the browser — in order to reduce browser-side bundle size.) (@frandiox I don't know if/how Hydrogen goes around that problem but let me know if you are interested in such modifier; I can share/publish it.) Note that both There are probably other use cases for custom modifiers we are currenlty not forseeing. Looking forward to ship |
The reason I propose to start with a plugin on userland is that
And then if the community come up with a good interface that fits most of the needs, we could port it back to the core and released it as a part of v3.0 to only introduce breaking changes one. |
I'm guessing you mean custom modifiers in general, since this PR in itself is trivial. The trick
This PR only adds 3 LOCs to core: https://github.com/vitejs/vite/pull/7209/files#diff-b43945059cc51ad52c7854aafa9e0435ecd8312e0740715d99fb6c6f5545cae1R154-R156.
This PR does not break anything. As for breaking the
In principle I agree but not for this specific case. The only thing my plugin does is to circumvent the fact that this PR is not merged. There is nothing the plugin enables me to experiment. It's actually the opposite: merging this PR fosters the Vite ecosystem to experiment with custom modifiers. Since this PR further enables custom modifiers. If we want to be highly conservative, we can even add a flag Bottom line:
|
I indeed tried to use glob modifiers a few months ago when implementing RSC the first time and was surprised that it wasn't supported (I believe this came up in a conversation with Evan on Hydrogen and RSC). We ended up finding a different approach but I think this feature could still help. I don't have a strong opinion on syntax or whether this should be in core or in a separate plugin first to test it out. That said, it is something that feels like it should be supported out of the box just like normal imports 🤔
I definitely am 👍 |
https://github.com/brillout/vite-plugin-ssr/blob/70d85039b806b1ee09ad1eca3d9e71e0968fe180/vite-plugin-ssr/node/plugin/extractStylesPlugin.ts — I added thorough comments but let me know if something's not clear. In case you play with it, make sure to use The plugin that re-implements the |
I'm closing this PR in favor of
@antfu That was a great idea :-). |
Description
Add support for custom modifiers for glob imports.
Additional context
Two frameworks that are being built on top of
vite-plugin-ssr
need this.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).