babichjacob / sapper-firebase-typescript-graphql-tailwindcss-actions-template

A template that includes Sapper for Svelte, Firebase functions and hosting, TypeScript and TypeGraphQL, Tailwind CSS, ESLint, and automatic building and deployment with GitHub Actions
MIT License
106 stars 14 forks source link

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

Closed Evertt closed 4 years ago

Evertt commented 4 years ago

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 commented 4 years ago

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 😦.

Evertt commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

So I had also reported this bug in the svelte language tools repo: https://github.com/sveltejs/language-tools/issues/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: https://github.com/sveltejs/language-tools/commit/50682449663795b3ef9d95adc64d037801368bc8

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 commented 4 years ago

This will be solved when https://github.com/babichjacob/sapper-postcss-template/commit/7f4d826cb4c286d3b70c4adb20a8c906b446a00d makes its way downstream here.

dummdidumm commented 4 years ago

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 commented 4 years ago

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

This has been here as https://github.com/babichjacob/sapper-firebase-typescript-graphql-tailwindcss-actions-template/commit/35d43ac3de18395b559722dbd0010fdd46f17854 since yesterday but I forgot to point it out

babichjacob commented 4 years ago

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 commented 4 years ago

We currently dynamically require for three things: