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

Svelte-preprocess default option problems #362

Closed
danitatt opened this issue May 21, 2021 · 20 comments · Fixed by #393
Closed

Svelte-preprocess default option problems #362

danitatt opened this issue May 21, 2021 · 20 comments · Fixed by #393
Assignees
Labels
enhancement New feature or request

Comments

@danitatt
Copy link

Hello everyone👋🏼 I really love svelte + svelte-preprocess for the ability to use the defaults option.

defaults: {
  markup: 'pug',
  script: 'coffee',
  style: 'sass'
}

But, when I use npm packages (e.g swiper/svelte), I get the error because the npm module is written in html/css/ js by default.
I get this same error when I use svelte-kit. Svelte-kit file generated without defaults and throws an error:
.svelte-kit/dev/generated/root.svelte

How can this problem be avoided? Maybe add option excludePaths? Where path node_modules & .svelte-kit exclude by default?

@PaHell
Copy link

PaHell commented Jun 7, 2021

Got the same problem. I'm using SvelteKit and everytime defaults: {} has been set, I get
`Error: /home/pat/Repos/heim/.svelte-kit/dev/generated/root.svelte:55:34
53| export let props_2 = null;
54|

55| setContext('svelte', stores);
-----------------------------------------^
56|
57| $: stores.page.set(page);
58| afterUpdate(stores.page.notify);

unexpected text ";

$"`

@kaisermann
Copy link
Member

kaisermann commented Jun 10, 2021

Hey, @danitatt 👋 We're discouraging the usage of the defaults options since it makes things much harder for tooling, so I'm not sure if adding a excludePaths would be the correct way forward. Adding an explicit lang=... is the most bug-guaranteed way to use other languages.

@benmccann
Copy link
Member

benmccann commented Aug 10, 2021

It turns out this also causes issues with Vite's optimizeDeps. SvelteKit forced that off until the latest release (next.146). Now that we've allowed it on people's projects are failing to compile with errors like:

src/routes/products/[slug]/[id].svelte:4:13: error: Expected "from" but found "{"
    4 │   import type { LoadInput, LoadOutput } from '@sveltejs/kit/types/page';
      ╵               ^

Vite only looks at the script blocks and doesn't try to read the default:
https://github.com/vitejs/vite/blob/5c0f0a362f669e3d38ddc51bfdd9d424a7c08feb/packages/vite/src/node/optimizer/scan.ts#L32

I would really like to be able to set the default and argued in favor of it originally, but it does seem to be causing problems. I think we should either have svelte-preprocess start warning or throwing an error if default is provided to force people off of it or we need to put it somewhere easier for tooling to read like in package.json instead of svelte.config.js

Putting it in package.json would fix Problem 1 (#350 (comment)). I don't understand Problem 2 and if there's anything that can fix it including an explicit <script lang="ts">. Perhaps we can discuss this issue more with dummdidumm when he returns from vacation

@kaisermann
Copy link
Member

have svelte-preprocess start warning or throwing an error if default

That we can do. Let's wait for @dummdidumm and decide the best move forward.

@AaronDDM
Copy link

@benmccann I am trying to understand the issue here, so have a few questions.

Is the issue here that vite is unable to distinguish what language is being used without an explicit lang tag? And is the assumption here that this is happening because projects are relying on different defaults (e.g. default ts), which vite is unaware of?

I am asking about the latter because I am getting the issue you described in your comment with no changes to my defaults and having the lang="ts" tag specified. After optimizeDeps was enabled, it seems like vite doesn't know that it needs to process TS code.

@benmccann
Copy link
Member

Yes, for the Vite issue you need to specify lang="ts" on every <script> and <script context="module"> block because Vite isn't aware of the default. If you still have issues after doing that, please file a new issue with a reproduction in the SvelteKit repo and I'll take a look

@dummdidumm
Copy link
Member

I'm in favor of logging a deprecation notice in the next minor and removing the functionality in the next major. While it feels nice to not have to write it every time, it introduces too many inconsistencies and maintenance burden all over the place, which hurts users of the default option more than it helps. The prettier/eslint plugin need to know it, language tools need to know it, now apparently vite needs to know it, too.
The main problem with the default is that most tooling still needs strictly synchronous paths, and the config file could be ESM which needs to be loaded asynchronously. Adding the default somewhere where loading is easier (package.json) would be a workaround but it would only shift the problem I think (every tool needs to update). Also, if we use the svelte field for that it would break other tooling which relies on this field to be a string.
Vue does not have this option AFAIK and I never saw an uproar about how badly they need it.

@jacob-8
Copy link

jacob-8 commented Aug 21, 2021

logging a deprecation notice

Yes, please do this soon! Would have saved me a lot of time trying to discern why builds on Vercel suddenly started failing but not on my local when I upgraded kit from v135 to v157. I couldn't find anything on this note anywhere until incrementally upgrading kit to v146 and reading the PR for it - sveltejs/kit#2137.

@kaisermann
Copy link
Member

There we go #393

@kaisermann
Copy link
Member

Deprecation notice released in 4.8.0 🎉

@kaisermann kaisermann self-assigned this Aug 23, 2021
@kaisermann kaisermann added the enhancement New feature or request label Aug 23, 2021
@girishnuli
Copy link

The examples are all using rollup. Any idea how the below config can be modified to work with snowpack?. If we remove the defaults and just use script lang="ts" in all the components, snowpack keeps throwing an "Unexpected token" error. Thanks.

module.exports = { preprocess: autoPreprocess({ defaults: { script: "typescript", }, postcss: true, }), };

@benmccann
Copy link
Member

That sounds like a bug that needs to be filed with snowpack

@girishnuli
Copy link

Possibly. However, is there an alternate option for typescript that can be added instead of defaults, which can make the deprecation notice go away?

@dummdidumm
Copy link
Member

No, the whole point of the deprecation message is to nudge you into not using it anymore, and there is no replacement, you should do lang="ts" in each file instead (note: if you have a module script and instance script, they both need that attribute) as this is way more robust for all the tooling out there (except snowpack apparently)

@Salman2301
Copy link

Using

<script lang="typescript">

does not work but using

<script lang="ts">

Works.
I think both ts and typescript should work

@girishnuli
Copy link

girishnuli commented Sep 20, 2021 via email

@outloudvi
Copy link

outloudvi commented Sep 28, 2021

@girishnuli, would you be able to add a line to node_modules/@snowpack/plugin-svelte/plugin.js to see which file triggers the error, and potentially have a look at it?

// probably near line 175 at @snowpack/plugin-svelte==3.7.0
console.log("---", filePath); // <-- add this to check which file has a problem on this
const compiled = svelte.compile(codeToCompile, finalCompileOptions);
const {js, css} = compiled;

(for myself, I realized one file where lang="ts" is absent in this way.)

@girishnuli
Copy link

@outloudvi Turns out there was just one file that was missing lang="ts" and causing the build to fail. Your solution helped identify the file and fix the issue. Thanks a ton for the help.

wlach added a commit to mozilla/glean-dictionary that referenced this issue Nov 26, 2021
* Move away from making scss the default, instead specify it manually
  ("defaults" are now discouraged: sveltejs/svelte-preprocess#362)
* Import only the parts of protocol we actually need for each component,
  this speeds up the build by a fair chunk (50.25 -> 18.85 seconds on
  my MacBook) and helps pave the way to #682.
wlach added a commit to mozilla/glean-dictionary that referenced this issue Nov 29, 2021
* Move away from making scss the default, instead specify it manually
  ("defaults" are now discouraged: sveltejs/svelte-preprocess#362)
* Import only the parts of protocol we actually need for each component,
  this speeds up the build by a fair chunk (50.25 -> 18.85 seconds on
  my MacBook) and helps pave the way to #682.
wlach added a commit to mozilla/glean-dictionary that referenced this issue Nov 29, 2021
* Move away from making scss the default, instead specify it manually
  ("defaults" are now discouraged: sveltejs/svelte-preprocess#362)
* Import only the parts of protocol we actually need for each component,
  this speeds up the build by a fair chunk (50.25 -> 18.85 seconds on
  my MacBook) and helps pave the way to #682.
@HNazmul-X
Copy link

what the ... default is depcreated ... what we have to do now

@dummdidumm
Copy link
Member

Add lang=".." to every script or style tag where it's needed.

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

Successfully merging a pull request may close this issue.