Skip to content

[fix] support using arrays for kit.vite.resolve.alias#2328

Merged
benmccann merged 8 commits into
sveltejs:masterfrom
bluwy:gh-2319
Aug 31, 2021
Merged

[fix] support using arrays for kit.vite.resolve.alias#2328
benmccann merged 8 commits into
sveltejs:masterfrom
bluwy:gh-2319

Conversation

@bluwy
Copy link
Copy Markdown
Member

@bluwy bluwy commented Aug 30, 2021

Fixes #2319

Thanks to @JeanJPNM for pointing out the additional places to update.

Notes:

  1. Maybe we can do the entire config merge as a Vite plugin per this PR's implementation?
  2. I can't put packages/kit/test/global.d.ts into packages/kit/test/ambient.d.ts because the latter has export {} in it, making it a TS module.

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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Aug 30, 2021

🦋 Changeset detected

Latest commit: 33bccef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann
Copy link
Copy Markdown
Member

thanks for the fix! it does feel a bit weird to me to do config merging in two different ways with this PR as it is right now. I would lean towards saying we either make the existing SvelteKit config merging work with regexes or do it with a Vite plugin for the whole Vite config blob (not sure if it'd be the same plugin or not, but I was also planning on adding a Vite plugin in #2232 to provide the SvelteKit middleware to the Vite server after vitejs/vite#4640 is merged)

@dominikg
Copy link
Copy Markdown
Contributor

i'm in favor of using vite plugins to provide additional config, note that you cannot add vite-plugin-svelte via a config hook though (plugins are final before the hooks are called). But everything else can be done via one or multiple sveltekit:config plugins with respective hooks.

@benmccann
Copy link
Copy Markdown
Member

hmm, I wonder if it just makes things more complicated. we'll still need the sveltekit config merging for non-vite stuff. I wonder if it wouldn't just be simpler to fix the sveltekit config merging to handle regexes instead. it already handles functions, so shouldn't be much different from that

@JeanJPNM
Copy link
Copy Markdown
Contributor

The problem isn't because of the regexes, is because the alias option had an array as a value, wich cannot be merged with an object.

Here is how it should be:

resolve: {
alias: Array.isArray(alias)
? [
{
find: '$app',
replacement: path.resolve(`${this.dir}/runtime/app`)
},
{
find: '$lib',
replacement: this.config.kit.files.lib
}
]
: {
$app: path.resolve(`${this.dir}/runtime/app`),
$lib: this.config.kit.files.lib
}

But this is a lot more verbose.

@JeanJPNM
Copy link
Copy Markdown
Contributor

I think we should create a plugins and make plugins for general config, dev and build, to avoid this kind of code duplication.

@JeanJPNM
Copy link
Copy Markdown
Contributor

The options I see are:

  • Hard coded replace
  • Anonymous plugins
  • Named plugins
  • Function specifically made to merge aliases

@benmccann
Copy link
Copy Markdown
Member

ah, thanks for clarifying. it looks like it was handled correctly in dev, but not in build 😝

I see that plugins.svelte.emitCss is also handled differently between the two and I have no idea why - that also looks like it might be broken in some way

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 31, 2021

Looking back now, I think adjusting the deep_merge to take into account of of resolve.alias would be cleaner. The extra plugin do seem off, but I think it's the safer choice in the long run if we do let Vite handle the config merging for us. resolveConfig might be handy too. I can look into that soon, but for now I'll refactor the code to not use a Vite plugin.

Update: There's also the mergeConfig function exported from vite.

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 31, 2021

Updated to have deep_merge take into account of resolve.alias, instead of using Vite plugins.

Comment thread packages/kit/test/apps/basics/src/routes/alias/_tests.js Outdated
@benmccann
Copy link
Copy Markdown
Member

awesome! this looks great! thanks so much 😄

@benmccann benmccann merged commit 3d4172c into sveltejs:master Aug 31, 2021
@bluwy bluwy deleted the gh-2319 branch August 31, 2021 16:57
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.

How to use kit.vite.resolve.alias if SvelteKit doesn't not allow this config?

4 participants