Skip to content

feat: support loading ts config files in esm contexts#249

Merged
ai merged 3 commits into
postcss:mainfrom
brc-dd:main
Nov 30, 2023
Merged

feat: support loading ts config files in esm contexts#249
ai merged 3 commits into
postcss:mainfrom
brc-dd:main

Conversation

@brc-dd

@brc-dd brc-dd commented Aug 11, 2023

Copy link
Copy Markdown
Contributor

Notable Changes

Commit Message Summary (CHANGELOG)

support loading ts config files in esm contexts and add support for .cts and .mts files

Type

  • Feature

SemVer

  • Breaking Change (:label: Major)

Issues

Checklist

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)

@ai

ai commented Aug 11, 2023

Copy link
Copy Markdown
Member

Can you fix test coverage?

@brc-dd

brc-dd commented Aug 12, 2023

Copy link
Copy Markdown
Contributor Author

Updated!

@brc-dd brc-dd marked this pull request as draft August 12, 2023 04:21
@brc-dd brc-dd marked this pull request as draft August 12, 2023 04:21
@brc-dd brc-dd marked this pull request as ready for review August 12, 2023 04:48
@yunsii

yunsii commented Oct 13, 2023

Copy link
Copy Markdown

What a great work, any progress here?

@ai

ai commented Oct 13, 2023

Copy link
Copy Markdown
Member

@yunsii can you test this branch on big project? If it works, I can try to merge and release it.

@yunsii

yunsii commented Oct 14, 2023

Copy link
Copy Markdown

@yunsii can you test this branch on big project? If it works, I can try to merge and release it.

Sure, I can test it, but I use postcss config with simple config, only use plugins option for now. Test coverage is not good enouph to prove it?

@ai

ai commented Nov 8, 2023

Copy link
Copy Markdown
Member

@yunsii

Sure, I can test it, but I use postcss config with simple config, only use plugins option for now. Test coverage is not good enouph to prove it?

Yes, we do not need more (we just need to be sure that config works in general, anyway different keys parsing is the same for all config types).

@yunsii

yunsii commented Nov 8, 2023

Copy link
Copy Markdown

Since it is so, I will try to test in this weekend.

@neoReuters

neoReuters commented Nov 8, 2023

Copy link
Copy Markdown

So just to confirm,

a postcss.config.ts file with the following configuration:

import type { Config } from 'postcss-load-config';
import tailwindcss from 'tailwindcss';
import autoprefixer from 'autoprefixer';

export default {
  plugins: [
    tailwindcss(),    
    autoprefixer,
  ],
} satisfies Config;

Should work with this fix?

@ai

ai commented Nov 8, 2023

Copy link
Copy Markdown
Member

@neoReuters as I understand, yes

@brc-dd

brc-dd commented Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

I should also test this once with Deno. I don't think jiti supports that. With Node and Bun it should work fine, but Deno is different story. We can probably bypass the loader in Deno (or probably for everything where native import doesn't throw) 👀

@ai

ai commented Nov 9, 2023

Copy link
Copy Markdown
Member

@brc-dd can you also rebase PR?

We can merge it when somebody will test it in big production project.

@brc-dd brc-dd marked this pull request as draft November 11, 2023 09:42
@brc-dd brc-dd marked this pull request as ready for review November 16, 2023 19:26
@brc-dd

brc-dd commented Nov 16, 2023

Copy link
Copy Markdown
Contributor Author

Updated the code to lazy load jiti -- use native import and only fallback to jiti if that fails. This enables better support for .postcssrc.ts files in Deno and Bun. But yeah, there can be some hidden issues (like with interop default). I'd suggest to publish a pre-release version like 5.0.0-beta.0, and ask people to use that version (preferably using something like npm/pnpm overrides so we can see how it plays with other tools).

@ai

ai commented Nov 16, 2023

Copy link
Copy Markdown
Member

I'd suggest to publish a pre-release version like 5.0.0-beta.0, and ask people to use that version (preferably using something like npm/pnpm overrides so we can see how it plays with other tools).

We don’t have enough active followers for that :(

@kingyue737

Copy link
Copy Markdown

Test this PR in my template project kingyue737/vitify-admin#25 and it works (CI failed because I install postcss-load-config from github:brc-dd/postcss-load-config, I don't know how to make it pass in github CI). But I need to rename postcss.config.ts to postcss.config.mts, even though type: module has been set in package.json.

@brc-dd

brc-dd commented Nov 20, 2023

Copy link
Copy Markdown
Contributor Author

Ah, with vite 5 it won't work properly. In vite, we are currently bundling this module with a custom patch.

@ai

ai commented Nov 28, 2023

Copy link
Copy Markdown
Member

In vite, we are currently bundling this module with a custom patch.

What patches do you use? We can move them to the core?

Comment thread src/index.js Outdated
Comment thread package.json
@ai

ai commented Nov 28, 2023

Copy link
Copy Markdown
Member

@brc-dd will we be able to use JSON .postcssrc but import there TS ESM plugins like:

{
  "plugins": {
    "./postcss/my-custom-plugin.ts": {}
  }
}

@brc-dd

brc-dd commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

What patches do you use? We can move them to the core?

Ah, that's same as fa058dd. But during vite 5 release, that wasn't published (mts / ts esm is still not supported). Also there are some patches for ts-node (need to tell rollup that it's not required at top and don't bundle that) -- those won't make sense in the core. That's why normal pnpm overrides for this module won't work for vite. Similar case for Next.js too, they also patch/bundle this module I guess.

@brc-dd brc-dd marked this pull request as ready for review November 30, 2023 04:34
@brc-dd brc-dd requested a review from ai November 30, 2023 04:34
@brc-dd

brc-dd commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Do we need to add some changes?

Yeah, had to move that ts extension test above importing jiti. Updated that now.

@ai ai merged commit 6c4e3e0 into postcss:main Nov 30, 2023
@brc-dd

brc-dd commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Supporting ESM (even without TS) will need big refactor - will have to make everything async to use dynamic imports, etc. Do we need that?

I need it in my project :D.

I'll try to create a PR separately. It won't be a breaking change as our exported function is async anyway.

@brc-dd

brc-dd commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

@ai Any specific reason for making postcss required peer dep in 75364b4? Initially, it was discussed here I guess - vitejs/vite#7495 (comment)

I'm fine with the change because mostly we already have postcss installed. But curious if there are people using this just to load config and pass it to some other tool and they don't have a direct dependency on postcss, this might gonna start giving warnings on things like yarn berry or pnpm with autoInstallPeers off / or on pnpm@7 or lower

GitHub search results - https://github.com/search?q=/%22postcss-load-config%22/++path:/package%5C.json/++NOT+/%22postcss%22/+NOT+is:fork+NOT+is:archived&type=code

@ai

ai commented Nov 30, 2023

Copy link
Copy Markdown
Member

Any specific reason for making postcss required peer dep

I removed it because I didn’t find a reason of loading config without having postcss.

If we will have real use case, I will revert change.

@brc-dd

brc-dd commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

If we will have real use case

People doing something like this - https://github.com/didi/mand-mobile/blob/4611c9bf5dda15b244a0da768fde20b80cb997b2/build/rollup/rollup-plugin-example-config.js#L37-L56 (passing plugins/options to rollup-plugin-css)

@ai

ai commented Nov 30, 2023

Copy link
Copy Markdown
Member

People doing something like this

Sure. Reverted.

Comment thread src/index.js
@ai

ai commented Nov 30, 2023

Copy link
Copy Markdown
Member

I'll try to create a PR separately. It won't be a breaking change as our exported function is async anyway.

Is it OK if I will wait this PR before the release?

I updated code style. The project is ready for forking again.

@brc-dd

brc-dd commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Is it OK if I will wait this PR before the release?

I'll try to create one by this week 👍

@ai

ai commented Dec 1, 2023

Copy link
Copy Markdown
Member

Released in 5.0.

@privatenumber

privatenumber commented Jan 20, 2024

Copy link
Copy Markdown
Contributor

Would yall be open to supporting tsx as a fallback loader too? Would be less dependencies in projects that use esbuild (e.g. Vite).

@brc-dd

brc-dd commented Jan 20, 2024

Copy link
Copy Markdown
Contributor Author

@privatenumber Does tsx provide a way to require specific files? Or register/unregister loaders in node 18?

@privatenumber

Copy link
Copy Markdown
Contributor

I'll look into supporting both of those!

@brc-dd

brc-dd commented Jan 20, 2024

Copy link
Copy Markdown
Contributor Author

Also, the way it's currently implemented, it already supports tsx. Like if you run tsx file_that_loads_config.js or node --import tsx file_that_loads_config.js it will work fine even if you don't have jiti installed. Same with anything like tsimp or tsm. But to make it work for node.js without any loader, it'll need something that can require TS on demand.

@privatenumber

Copy link
Copy Markdown
Contributor

I noticed, but

  1. I would prefer not to need to use tsx for running future versions of Vite only for the PostCSS config

  2. This is an interesting but valid use-case, and I'd like tsx to be a more versatile utility to handle something like this as well

@brc-dd

brc-dd commented Jan 20, 2024

Copy link
Copy Markdown
Contributor Author

Also, if you're supporting something like importUsingTsx then can you also add dependency tracking? It's not needed for postcss, but many of the projects in vite ecosystem are using hacky way of using vite's loadConfigFromFile for this. There vite is returning esbuild's metafile that has list of deps. For example, in vitepress we use that for loading config, path and data loader files, and need to reload if they or any of their dependent files are changed. In contrast, Nuxt is using jiti, but that doesn't support this and hence if you change any dependent file of config it doesn't trigger reload. You need to manually restart the process.

@karlhorky

karlhorky commented Feb 11, 2024

Copy link
Copy Markdown

Thanks for the release postcss-load-config@5.0.0 with full ESM support in postcss.config.ts with "type": "module" @brc-dd and @ai 🙌

@brc-dd

brc-dd commented Feb 11, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the release postcss-load-config@5.0.0 with full ESM support in postcss.config.ts with "type": "module" brc-dd and ai 🙌

Hey man, we understand that you're happy about the added support. But please avoid pinging and posting the same message on each thread. We already have to go through a bunch of notifications on daily basis and these just add noise. Prefer using emoji reactions instead.

@karlhorky

Copy link
Copy Markdown

Understood, I'll try to reduce the pinging and notifications. Sorry.

For background, what I'm aiming to do is make it easier to easily find the exact version that support was added in when visiting this issue from a search engine eg. Google - because this relates to a bug that will probably affect people for some time.

I'll try to find a middle path that still does this but reduces the noise.

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.

Add ESM support to typescript config files TypeScript doesn't work with "type": "module" in package.json

7 participants