-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: support loading ts config files in esm contexts #249
Conversation
Can you fix test coverage? |
Updated! |
What a great work, any progress here? |
@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 |
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). |
Since it is so, I will try to test in this weekend. |
So just to confirm, a 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? |
@neoReuters as I understand, yes |
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) 👀 |
@brc-dd can you also rebase PR? We can merge it when somebody will test it in big production project. |
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). |
We don’t have enough active followers for that :( |
Test this PR in my template project kingyue737/vitify-admin#25 and it works (CI failed because I install |
Ah, with vite 5 it won't work properly. In vite, we are currently bundling this module with a custom patch. |
What patches do you use? We can move them to the core? |
@brc-dd will we be able to use JSON {
"plugins": {
"./postcss/my-custom-plugin.ts": {}
}
} |
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. |
Yeah, had to move that ts extension test above importing jiti. Updated that now. |
I'll try to create a PR separately. It won't be a breaking change as our exported function is async anyway. |
@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 |
I removed it because I didn’t find a reason of loading config without having If we will have real use case, I will revert change. |
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) |
Sure. Reverted. |
Is it OK if I will wait this PR before the release? I updated code style. The project is ready for forking again. |
I'll try to create one by this week 👍 |
Released in 5.0. |
Would yall be open to supporting |
@privatenumber Does tsx provide a way to require specific files? Or register/unregister loaders in node 18? |
I'll look into supporting both of those! |
Also, the way it's currently implemented, it already supports tsx. Like if you run |
I noticed, but
|
Also, if you're supporting something like |
Thanks for the release |
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. |
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. |
Notable Changes
Commit Message Summary (CHANGELOG)
Type
SemVer
Issues
Checklist