Skip to content
This repository has been archived by the owner on Apr 19, 2021. It is now read-only.

Bug using svelte-check when having tailwind's experimental features turned on #18

Closed
Evertt opened this issue Aug 26, 2020 · 10 comments
Closed
Labels
bug Something isn't working enhancement New feature or request upstream This can only be solved by an upstream project's changes

Comments

@Evertt
Copy link

Evertt commented Aug 26, 2020

If you write export const experimental = "all" in tailwind.config.js then you get a huge upgrade to the @apply rule. Namely then you can suddenly write @apply hover:bg-red-200, which you couldn't do before.

Unfortunately however, with how this template is currently set up svelte-check will complain that that is not valid syntax. The reason for that is that svelte.config.js doesn't actually use postcss.config.js and therefor it doesn't use tailwind.config.js and therefor svelte-check is not aware that tailwind's experimental feature has been turned on.

I've been able to fix it in my own repo. It wasn't quite as simple as just requiring postcss.config.js, because all those other config files were written in ES6 format, while svelte.config.js was written in Node format and that makes it impossible to use require(). So I had to convert all those config files to Node format.

Right now I'm a bit too lazy to open a PR, but here's the commit in which I fixed the problem for myself. One extra benefit of how I have it now, is that svelte.config.js is the one and only source of truth regarding preprocessing configuration.

@babichjacob
Copy link
Owner

If you write export const experimental = "all" in tailwind.config.js then you get a huge upgrade to the @apply rule. Namely then you can suddenly write @apply hover:bg-red-200, which you couldn't do before.

Enabling experimental features will introduce warnings that may confuse people into thinking this project doesn't work.

Thanks for hunting down that this is a problem and fixing it. It's clearly an improvement, but I hate CommonJS a lot and don't know for sure if I'll spend the time refactoring all 3 templates (and my own sites that use them) to use it to fix validation and Tailwind CSS IntelliSense. I'd sooner hope for these tools to support ES imports [does VS Code run extensions as CommonJS or something?] than downgrade.

Hard situation 😦.

@babichjacob babichjacob added bug Something isn't working enhancement New feature or request upstream This can only be solved by an upstream project's changes labels Aug 26, 2020
@Evertt
Copy link
Author

Evertt commented Aug 26, 2020

I also hate CommonJS, it's ugly. However, these experimental features of tailwind will soon be default features. And especially the upgraded @apply feature is gonna be very popular. So I'm afraid that if you don't switch those config files to CommonJS then soon you won't be able to use the latest version of tailwindcss anymore.

I don't know if the makers of svelte-check can make it work with ES6 modules, but if I were you I wouldn't (want to) count on that.

@babichjacob
Copy link
Owner

babichjacob commented Aug 26, 2020

However, these experimental features of tailwind will soon be default features. And especially the upgraded @apply feature is gonna be very popular.

Not for certain. They're allowed to change, and because they're experimental features, should be opt-in.

So I'm afraid that if you don't switch those config files to CommonJS then soon you won't be able to use the latest version of tailwindcss anymore.

Only for validation (which I assume can be overridden with whatever the svelte-check ignore comment is); the features still work.

@Evertt
Copy link
Author

Evertt commented Aug 26, 2020

Not for certain.

You think there's a chance that the upgraded @apply won't be made default in the next version of tailwindcss? To me it seems basically guaranteed. They themselves said it's the most requested feature ever and they've finally been able to make it work.

and because they're experimental features, should be opt-in

Sure, I'm not asking you to turn on those experimental features in your template. Because I agree, they should be opt-in. I'm just asking you to prepare your template such that when someone does opt in, that everything just works as they expect.

Only for validation (which I assume can be overridden with whatever the svelte-check ignore comment is); the features still work.

True, but looking at the trade-off I'd say that having validation working is much more valuable than having a few config files in ES6 format which we find prettier code to look at.

@Evertt
Copy link
Author

Evertt commented Aug 27, 2020

So I had also reported this bug in the svelte language tools repo:
sveltejs/language-tools#484

And their solution was to add a note to their documentation to make clear that you can only use CommonJS in your config files:
sveltejs/language-tools@5068244

Note that within your config files you can only use node-syntax, things like import ... or export const ... are not allowed.

In other words, I think this confirms that they have no intention to make svelte check work with a ES6 config file.

@babichjacob
Copy link
Owner

This will be solved when babichjacob/sapper-postcss-template@7f4d826 makes its way downstream here.

@dummdidumm
Copy link

In other words, I think this confirms that they have no intention to make svelte check work with a ES6 config file.

It's more like a ESM-CommonJS-thing that's happening. The problem is that our code is CommonJS and is using require to synchronously load the configs. Using the async import is not an option. If you can give me any pointers how I can make this work, I'm happy to take a look.

@babichjacob
Copy link
Owner

This will be solved when babichjacob/sapper-postcss-template@7f4d826 makes its way downstream here.

This has been here as 35d43ac since yesterday but I forgot to point it out

@babichjacob
Copy link
Owner

babichjacob commented Aug 28, 2020

In other words, I think this confirms that they have no intention to make svelte check work with a ES6 config file.

It's more like a ESM-CommonJS-thing that's happening. The problem is that our code is CommonJS and is using require to synchronously load the configs. Using the async import is not an option. If you can give me any pointers how I can make this work, I'm happy to take a look.

I looked in to how rollup is able to load ES rollup.config.jss, but it's unfortunately await import too.

Do you guys have an issue there that already exists or can you give a short explanation here for what limits you from being async? [Disclaimer: I won't have the resources to help fix it]

@dummdidumm

@dummdidumm
Copy link

We currently dynamically require for three things:

  • User's Svelte version/module. That's inside some async context so it would'nt be a problem there
  • User's Prettier version/module. Also inside async context.
  • The preprocess config. This one is in a synchronous path, because it's possible to get invoked from the typescript language service which is all synchronous, so no way to getting that asynchronous. The only thing we could do is to change the lookup of a svelte.config.js so that it's only done once on startup, not per file. But this would limit people to only have one svelte.config.js per project, so this would be kind of a breaking change - although I doubt it will break anyone because noone has more than one svelte.config.js in his project.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request upstream This can only be solved by an upstream project's changes
Projects
None yet
Development

No branches or pull requests

3 participants