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: backward compat for setting values in resolvedConfig hook #17947

Conversation

sapphi-red
Copy link
Member

Description

Vike sets build.emitAssets in resolvedConfig hook and that was working in v5, but in v6 this is now ignored.
I guess we don't support updating config values in resolvedConfig hook, but we don't explicitly document anywhere that we don't support that, too (the types are shallow readonly rather than deep readonly...).
I'm not sure if we should merge this PR, but this PR would make ecosystem-ci pass for Vike.

Copy link

stackblitz bot commented Aug 26, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red
Copy link
Member Author

/ecosystem-ci run vike

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on ed0e800: Open

suite result latest scheduled
vike failure success

@sapphi-red
Copy link
Member Author

The remaining failing test expects a full-reload to happen when a SSR module is edited even if that file is not imported from the client side. This is expected to fail since v6 and requires changes on the framework side, am I right?

@patak-dev
Copy link
Member

/ecosystem-ci run vike

@patak-dev
Copy link
Member

I guess we don't support updating config values in resolvedConfig hook, but we don't explicitly document anywhere that we don't support that, too (the types are shallow readonly rather than deep readonly...).

Modifying values in ResolvedConfig should work mostly as before (except for these new experimental options). I think it is good to merge this PR so we patch them.

But in general, we should discuss what to do and document things up. One of the issues is that changes in ResolvedConfig could be missed if the plugin has cached values (we at least did this internally for root and other values). Or if there is any init code on ResolvedConfig that relies on the config being done in a plugin.

I think even if the types were not completely forbidding changes, the docs were more clear:

configResolved: Called after the Vite config is resolved. Use this hook to read and store the final resolved config. It is also useful when the plugin needs to do something different based on the command being run.

We didn't respect this ourselves though. For example in the plugin legacy.
Maybe we need a beforeConfigResolved hook.

The issue with environments config is that changes to the root values will not be reflected on the resolved environment options. My take here is that we should see why plugins need to do changes at configResolved time and offer a way to do the same in previous hooks.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 03c02ee: Open

suite result latest scheduled
vike failure success

@patak-dev
Copy link
Member

The remaining failing test expects a full-reload to happen when a SSR module is edited even if that file is not imported from the client side. This is expected to fail since v6 and requires changes on the framework side, am I right?

@sheremet-va @hi-ogawa should we add an option to implement this? If there is a server only module, we could have a server.reloadClientOnSsrOnlyChange flag set to true by default (for back compat) that triggers a full-reload for the client. Maybe this would also help with the remix issue 🤔

@hi-ogawa
Copy link
Collaborator

should we add an option to implement this? If there is a server only module, we could have a server.reloadClientOnSsrOnlyChange flag set to true by default (for back compat) that triggers a full-reload for the client. Maybe this would also help with the remix issue 🤔

Making up full-reload event is probably not enough for Remix case since it's expecting import.meta.hot.accept callback on client side for server only change.

@sapphi-red sapphi-red changed the title fix: backward compat for setting build.emitAssets in resolvedConfig hook fix: backward compat for setting values in resolvedConfig hook Aug 26, 2024
@patak-dev patak-dev merged commit 34308fa into vitejs:v6/environment-api Aug 26, 2024
8 of 9 checks passed
@sapphi-red sapphi-red deleted the fix/ssremitassets-resovledconfig-backward-compat branch September 2, 2024 01:52
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.

3 participants