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

Logging for overridden config in vite does not include cors options for dev and preview #12574

Open
fnimick opened this issue Aug 12, 2024 · 7 comments
Labels

Comments

@fnimick
Copy link
Contributor

fnimick commented Aug 12, 2024

Describe the bug

Some config options are set by sveltekit and are not warned if they are overriding user-supplied vite options, though other options do have warnings enabled.

For example, the svelte plugin explicitly sets server.cors and preview.cors to { preflightContinue: true }. This fails to report an overridden config if the user configuration was either cors: false or cors: { preflightContinue: false }.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-l9njmw

This has server.cors set to { preflightContinue: false } and preview.cors set to false. Both are overriden with no warning.

Logs

Resolved config:

  vite:config   server: {
  vite:config     preTransformRequests: true,
  vite:config     cors: { preflightContinue: true },
  vite:config     fs: {
  vite:config       strict: true,
  vite:config       allow: [Array],
  vite:config       deny: [Array],
  vite:config       cachedChecks: undefined
  vite:config     },
  vite:config     sourcemapIgnoreList: [Function: sourcemapIgnoreList],
  vite:config     watch: { ignored: [Array] },
  vite:config     middlewareMode: false
  vite:config   },
  vite:config   preview: {
  vite:config     port: undefined,
  vite:config     strictPort: undefined,
  vite:config     host: undefined,
  vite:config     https: undefined,
  vite:config     open: undefined,
  vite:config     proxy: undefined,
  vite:config     cors: { preflightContinue: true },
  vite:config     headers: undefined
  vite:config   },


### System Info

```Shell
System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.0 => 3.2.3 
    @sveltejs/kit: ^2.0.0 => 2.5.21 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.1.1 
    svelte: ^4.2.7 => 4.2.18 
    vite: ^5.0.3 => 5.4.0

Severity

annoyance

Additional Information

No response

@fnimick
Copy link
Contributor Author

fnimick commented Aug 12, 2024

I suspect fixing this may be as simple as adding the preflightContinue keys to the enforced_config at

const enforced_config = {

@dominikg
Copy link
Member

dominikg commented Aug 12, 2024

it looks like that was added in https://github.com/sveltejs/kit/pull/9619/files but not sure if intentional.

@benmccann do you remember why that cors setting was added to the config modifications?

different PR, see below

@fnimick
Copy link
Contributor Author

fnimick commented Aug 12, 2024

That commit appears to be simply moving the cors: { preflightContinue: true } option. The options were added in #8731

I'm not suggesting removing these options, but sveltekit overriding user provided options should be included in the vite warning log, similar to other options that are overridden.

I believe preflightContinue is necessary for the vite default cors handler to pass requests through to OPTIONS endpoints in dev and preview.

@dominikg
Copy link
Member

I am not sure overriding user provided options is the right way here. having cors disabled is something users might want even in dev/preview. The example in #8731 (good find) indicates that explicit false should be.

cc @eltigerchino

@dominikg
Copy link
Member

as a workaround, you can add a vite plugin with a config hook after the sveltekit plugin to reset it to false:

plugins:[sveltekit(),{
  name: 'vite-plugin-no-cors-config',
  config(){
    return {server:{cors: false}, preview:{cors: false}}
  }
}]

@eltigerchino
Copy link
Member

eltigerchino commented Aug 13, 2024

Yeah, I’m highly in favour of letting user config take precedence. I think the only reason I didn’t do so in that PR was because the preflight continue option managed to preserve existing behaviour while allowing the options endpoint to work

@fnimick
Copy link
Contributor Author

fnimick commented Aug 20, 2024

A proposal, re: "letting user config take precedence":

  • if server.cors or preview.cors is unset (undefined): set to { preflightContinue: true }
  • if server.cors or preview.cors is an object, and preflightContinue is unset: set to true
  • if server.cors or preview.cors is true, or server.cors.preflightContinue or preview.cors.preflightContinue is false, do not modify config (and potentially warn in the log that OPTIONS endpoints may not work due to vite handling requests and not passing through to sveltekit)
  • If server.cors or preview.cors is false, do not modify config

Is this behavior too complicated? It seems to me that it would be the best DX where OPTIONS handlers will 'just work' but vite cors handling will not be enabled if the user is explicitly trying to disable it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants