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

chore(deps)!: update postcss-load-config to v6 #15235

Merged
merged 16 commits into from
Oct 28, 2024

Conversation

brc-dd
Copy link
Contributor

@brc-dd brc-dd commented Dec 4, 2023

Description

Bump postcss-load-config to v5. It added support for ESM + TS postcss configs (postcss/postcss-load-config#249). closes #15234 closes #15745 (and other issues that were closed/locked as they were upstream issues).

Additional context

It now needs jiti instead of ts-node for TS config files in Node.js. Might be a slightly breaking change for users?


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 4, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@brc-dd brc-dd marked this pull request as draft December 4, 2023 02:04
@brc-dd brc-dd marked this pull request as ready for review December 4, 2023 03:49
@brc-dd brc-dd marked this pull request as draft December 4, 2023 04:35
@brc-dd brc-dd marked this pull request as ready for review December 4, 2023 05:07
@bluwy
Copy link
Member

bluwy commented Dec 4, 2023

Yeah I think this is a breaking change and we can't bump this until v6. Unless we can still get ts-node working partially, but it doesn't seem likely now with postcss-load-config cutting a major.

Also for future reference, the renovate PR: #15234

@bluwy bluwy added this to the 6.0 milestone Feb 13, 2024
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) breaking change labels Feb 13, 2024
@bluwy bluwy linked an issue Feb 13, 2024 that may be closed by this pull request
7 tasks
@kuchta
Copy link

kuchta commented Apr 6, 2024

Hi guys,

Is there something blocking this PR to be part of final 6.0?

@patak-dev
Copy link
Member

@kuchta the PR is already in the v6 milestone. Would you comment on your use case so your comment gives value to this PR?

@kuchta
Copy link

kuchta commented Apr 8, 2024

@patak-dev Well, it's not part of the v6 alpha release, so I'm curious... My use case is using postcss.config.ts

@patak-dev
Copy link
Member

@kuchta I see. We're going to use the v6 alpha series to let the ecosystem test the Environment API branch. See:

@ArnaudBarre
Copy link
Member

For information I sent this PR to reduce bundle size: postcss/postcss-load-config#261

@brc-dd brc-dd changed the title chore(deps): update postcss-load-config to v5 chore(deps): update postcss-load-config to v6 Jun 2, 2024
@bluwy
Copy link
Member

bluwy commented Oct 4, 2024

I've updated the branch, but I don't think it's working correctly quite yet. Because postcss-load-config now requires tsx, jiti, yaml etc to be installed manually via peer deps, Vite also have to specify those peer deps to be able to transfer them to postcss-load-config.

The alternative is for postcss-load-config to import those dependencies from the base directory of where the config is located so Vite doesn't have to specify the peer deps, I'm not sure if that's something still on the table?

Also testing locally, it doesn't look like there's an error logged if postcss-load-config fails the load the config. If I do a manual log at https://github.com/brc-dd/vite-docs/blob/355e2f656ceea064855c4705e4db0ef6994e368c/packages/vite/src/node/plugins/css.ts#L1621, it shows this error only:

Error: No PostCSS Config found in: /Users/bjorn/Work/repros/vite-tailwind
    at file:///Users/bjorn/Work/repros/...

@kuchta
Copy link

kuchta commented Oct 4, 2024

Thank you @bluwy. If i want to try it, will I have to clone it, build it manually and then install it from build output directory or is it possible to install it directly from that branch using something like pnpm add -D https://github.com/brc-dd/vite-docs.git#bump-postcss-load-config? I've tried installing it directly by pnpm, but it fails with:

: pnpm add -D https://github.com/brc-dd/vite-docs.git#bump-postcss-load-config
 WARN  6 deprecated subdependencies found: @babel/plugin-proposal-private-methods@7.18.6, glob@7.2.3, inflight@1.0.6, rollup-plugin-terser@7.0.2, sourcemap-codec@1.4.8, workbox-google-analytics@7.0.0
Packages: +1
+
Progress: resolved 656, reused 577, downloaded 0, added 0, done
node_modules/.pnpm/@vitejs+vite-monorepo@https+++codeload.github.com+brc-dd+vite-docs+tar.gz+355e2f656ceea064855c4705e4db0ef6994e368c/node_modules/@vitejs/vite-monorepo: Running postinstall script...
 ELIFECYCLE  Command failed.

@bluwy
Copy link
Member

bluwy commented Oct 4, 2024

You have to clone it locally for now unfortunately, but we're working on making preview releases better soon. The flow that use is a little complicated but it goes like this:

  1. Build Vite locally (follow the contributing guide to set it up)
  2. In your project, (assuming using pnpm) add this in package.json:
     "pnpm": {
       "overrides": {
        "vite": "file:../../oss/vite/packages/vite"
       }
     },
    (file: ensures that it's not linked for pnpm. If you use other package managers, make sure it's not linked. It should be installed like normal packages in node_modules, which pnpm guarantees that with file:)
  3. Run pnpm i in your project to apply the overrides
  4. Test!

@kuchta
Copy link

kuchta commented Oct 4, 2024

Thank you @bluwy. Much appreciated 🙏🏻

bluwy
bluwy previously approved these changes Oct 24, 2024
@brc-dd
Copy link
Contributor Author

brc-dd commented Oct 24, 2024

Looks like __filename (here) is not transformed. We can adjust cjsPatchPlugin probably or just add another replacement specifically for postcss-load-config?

Because postcss-load-config now requires tsx, jiti, yaml etc to be installed manually via peer deps, Vite also have to specify those peer deps to be able to transfer them to postcss-load-config.

The alternative is for postcss-load-config to import those dependencies from the base directory of where the config is located so Vite doesn't have to specify the peer deps, I'm not sure if that's something still on the table?

Regarding this, how to do it 😅 I can probably upstream it, but I'm not sure how to "import those dependencies from the base directory"

Also, in vite 5, ts-node for postcss-load-config needed to be hoisted. If vite doesn't want to add it to peer deps, people using pnpm can manually configure hoist-patterns? (Also IIRC pnpm since v8, hoists everything by default. So most people won't be seeing this anyway?)

vite/.npmrc

Line 1 in 91a1acb

hoist-pattern[]=ts-node # package/vite: postcss-load-config

@bluwy
Copy link
Member

bluwy commented Oct 24, 2024

Looks like __filename (here) is not transformed. We can adjust cjsPatchPlugin probably or just add another replacement specifically for postcss-load-config?

I think adding the replacement directly would be best. We had removed the __filename shim entirely recently because before this PR there's no dependencies relying on it (#18349). So if there's only one case here I think we can replace directly.

Regarding this, how to do it 😅 I can probably upstream it, but I'm not sure how to "import those dependencies from the base directory"

I was thinking that it could resolve the dependency based on the found config file path, then import that resolved path. Resolving could use require.resolve() perhaps (if they're not ESM only). But I think looking back, it's not too bad to have Vite list the peer deps. I think I'm fine with it now unless someone else have opinions on this, because it may look like we're endorsing jiti/tsx/yaml.

Also, in vite 5, ts-node for postcss-load-config needed to be hoisted. If vite doesn't want to add it to peer deps, people using pnpm can manually configure hoist-patterns? (Also IIRC pnpm since v8, hoists everything by default. So most people won't be seeing this anyway?)

Hmm I actually thought ts-node did work ootb when installed without hoisting. If it didn't work and require the package manager to hoist it up before, that's an interesting point. Nonetheless it would be nice to have the dependencies work without hoisting though. (I'm not sure if pnpm hoists everything by default today)

@brc-dd
Copy link
Contributor Author

brc-dd commented Oct 24, 2024

Updated.

(I'm not sure if pnpm hoists everything by default today)

https://pnpm.io/npmrc#hoist-pattern

By default, all packages are hoisted - however, if you know that only some flawed packages have phantom dependencies, you can use this option to exclusively hoist the phantom dependencies (recommended).

@sapphi-red sapphi-red changed the title chore(deps): update postcss-load-config to v6 chore(deps)!: update postcss-load-config to v6 Oct 25, 2024
@sapphi-red
Copy link
Member

I added a commit that reduces the patch by using external option. Rollup makes require to be force required upfront, but not for dynamic imports, so we can use the external option.
Also I added a commit that adds the migration guide section.

@sapphi-red
Copy link
Member

I think looking back, it's not too bad to have Vite list the peer deps. I think I'm fine with it now unless someone else have opinions on this, because it may look like we're endorsing jiti/tsx/yaml.

I think it's fine. The peerDependencies / peerDependenciesMeta is a bit bloated now though 😅. If we want to declare when these dependencies are needed, I guess we can put peerDependenciesMeta[deps].note? (It can be any name that does not conflict with the fields package managers use) But I don't think we need to do it.

sapphi-red
sapphi-red previously approved these changes Oct 25, 2024
@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on f6e1641: Open

suite result latest scheduled
astro failure failure
nuxt failure success
redwoodjs failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-plugin-svelte failure failure
vitest failure failure

analogjs, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

@sapphi-red
Copy link
Member

Now that yaml is an optional peer dep, the package size is reduced by ~250kB and the compressed package size is reduced by 60kB 🎉

@bluwy
Copy link
Member

bluwy commented Oct 25, 2024

pnpm.io/npmrc#hoist-pattern

By default, all packages are hoisted - however, if you know that only some flawed packages have phantom dependencies, you can use this option to exclusively hoist the phantom dependencies (recommended).

Looks like you're right TIL! Strange that it seemed like I needed peer deps to make it work in the past, but if it turns out we don't, especially given ts-node was in the same spot too, maybe we can skip declaring the peer deps instead. Any thoughts on this @sapphi-red ?

@brc-dd
Copy link
Contributor Author

brc-dd commented Oct 25, 2024

Strange that it seemed like I needed peer deps to make it work in the past

Yeah earlier it was needed. pnpm changed the default behavior somewhere between v7-9.

I guess it's better to keep them in peer deps, because I'm not sure how yarn berry behaves with that. People might still need packageExtensions there to make it work?

@sapphi-red
Copy link
Member

I'll prefer to keep the peerDeps as it's probably more correct. But I'm fine with going without it as it was like that with ts-node.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with peer deps then

@sapphi-red sapphi-red merged commit 3a27f62 into vitejs:main Oct 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
6 participants