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

Use svelte.config.js's extensions for plugin-svelte input #1373

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

techniq
Copy link
Contributor

@techniq techniq commented Oct 20, 2020

Changes

Uses svelte.config.js's extensions for plugin-svelte input if available (and not overridden by plugin config). This will cut down on duplicated configuration.

Testing

I wasn't for certain how best to update the tests for this change.

Docs

Update input section of README

@vercel
Copy link

vercel bot commented Oct 20, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/8ek70b5vs
✅ Preview: https://snowpack-git-svelte-resolve-input-config.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

Can you share a link to where this is documented as correct in a svelte.config.js file? Does such docs even exist?

I'm +1 on this PR, but in the past we've been bitten by matching other plugins and then realizing that the Svelte team had a different recommended API/pattern.

@firefish5000
Copy link
Contributor

There is no such doc nor recommendation inside of language-tools from what I can tell. Rollup plugin is the only thing I am aware of with an extensions field, webpack plugin uses a completely different method for finding svelte files and vscode requires changes via settings to support different extensions. I love standards, but I don't think this has even been on a path where standardization was an option up to this point

@techniq
Copy link
Contributor Author

techniq commented Oct 20, 2020

@FredKSchott @firefish5000 When I look at the Webpack loader (https://github.com/sveltejs/svelte-loader#usage) I see it referencing extensions in the config, as well as in the mdsvex's docs. I guess the main discussion is whether svelte.config.js having extensions is recommended. I don't know if @Rich-Harris has any feedback. It seems like you have to duplicate the input/extensions in both places if not, which seems prone to error.

fwiw, I was migrating to snowpack from svite and it had this convention. I was actually baffled for a bit as to why my .svx files were not being handled by snowpack until I saw @firefish5000's PR.

@FredKSchott
Copy link
Owner

Okay, I'm feeling good about this then! +1 to merging by EOD today or tomorrow, in case @Rich-Harris has any last minute thoughts

@firefish5000
Copy link
Contributor

firefish5000 commented Oct 20, 2020

@techniq Note that webpack config only uses extensions for webpack's resolve, NOT for the svelte-loader which uses a test regex. Both must be configured for it to work. And while that sort of adds a standard, webpack's resolve is for everything webpack can import, not just svelte stuff (as it is not a svelte project to begin with). So in practice webpack would need something like

const svelteExtensions = ['.svelte','.svx']
  ...
  resolve: {
    alias: {
      svelte: path.resolve('node_modules', 'svelte')
    },
    // Note this is not equivalent to the svelte rollup plugin's `extensions` field as it must specify extensions
    // for all import statements webpack can resolve. Svelte is merely one of the things it can resolve
    extensions: ['.js','.ts','.css', ...svelteExtensions]
    mainFields: ['svelte', 'browser', 'module', 'main']
  },
  module: {
    rules: [
      ...
      {
        // This is the code for the svelte loader, it uses a regex. We could easily build this from the svelteExtensions,
        // but its certainly not a direct relationship as this syntax is more powerful at the cost of being one of the
        // causes for webpacks config hell
        test: /\.(svelte|svx)$/,
        exclude: /node_modules/,
        use: 'svelte-loader'
      }
      ...
    ]
  }

This could certainly be made to use extensions, but atm, neither language-tools nor the default config for svelte-webpack and svelte-rollup support loading from svelte.config.js. The main issue is it is a non-standard field that we would be imposing a (albeit intuitive, as I use it as well in the same way) expected value for in a config file we do not own.
I think it would be best to wait for input from the svelte team to see if they have any plans to standardize this in the config, or if they possibly are thinking of doing so in another way

@FredKSchott FredKSchott merged commit e10e724 into FredKSchott:master Oct 21, 2020
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