Skip to content

fix: update postcss-load-config to load PostCSS plugins based on their config file path#6856

Merged
patak-cat merged 6 commits intovitejs:mainfrom
fwouts:postcss-loader-upgrade
Mar 3, 2022
Merged

fix: update postcss-load-config to load PostCSS plugins based on their config file path#6856
patak-cat merged 6 commits intovitejs:mainfrom
fwouts:postcss-loader-upgrade

Conversation

@fwouts
Copy link
Copy Markdown
Contributor

@fwouts fwouts commented Feb 10, 2022

Description

This should fix #4000 as PostCSS plugins will now be loaded based on the PostCSS config file path, instead of the current working directory.

Additional context

See postcss/postcss-load-config#229 for a detailed explanation.

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.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Niputi Niputi added dependencies Pull requests that update a dependency file p3-minor-bug An edge case that only affects very specific usage (priority) labels Feb 10, 2022
@Niputi
Copy link
Copy Markdown
Contributor

Niputi commented Feb 10, 2022 •
edited
Loading

would you be able to add a test? either here or in the postcss plugin

@fwouts
Copy link
Copy Markdown
Contributor Author

fwouts commented Feb 11, 2022 •
edited
Loading

@Niputi I've now added a test, it required reconfiguring pnpm (see .npmrc) as the current configuration allows any package, including postcss-load-config, to access unlisted dependencies including tailwindcss, so I couldn't actually make my test fail :) See relevant documentation at https://pnpm.io/npmrc#hoist

What's interesting is that, after making this change, some other tests started to fail. Some of them because of unlisted dependencies (see changes to various package.json files), some others because of the bug that this PR fixes.

While I would have preferred to extract the .npmrc and package.json changes into a separate preliminary PR, it doesn't seem possible specifically because of the tests that would start failing (such as the tailwind tests).

Let me know what you think!

@patak-cat patak-cat added this to the 2.9 milestone Feb 11, 2022
patak-cat
patak-cat previously approved these changes Feb 11, 2022
View reviewed changes
@patak-cat
Copy link
Copy Markdown
Member

patak-cat commented Feb 11, 2022

LGTM, let's merge it in the 2.9 beta period to get some testing from other projects in the ecosystem.

@fwouts
Copy link
Copy Markdown
Contributor Author

fwouts commented Feb 11, 2022

Appreciate it!

Heads-up, there's some discussion in the linked PR about making this change a major release of postcss-load-config instead of a patch, so perhaps better hold off for another day before merging this (as we'd want to upgrade to 4.0.0 instead).

@fwouts
Copy link
Copy Markdown
Contributor Author

fwouts commented Feb 11, 2022

@patak-dev the decision was made to release v3.1.3 with a small additional fix instead of a new v4 major version.

I've updated the dependency now, so this is no longer blocked :)

@patak-cat patak-cat removed the on hold label Feb 11, 2022
@hpx7
Copy link
Copy Markdown
Contributor

hpx7 commented Feb 26, 2022

Anything holding up being able to merge this?

@patak-cat
Copy link
Copy Markdown
Member

patak-cat commented Feb 26, 2022

We are going to merge it next week when we start the 2.9 beta

@patak-cat patak-cat merged commit f02f961 into vitejs:main Mar 3, 2022
@fwouts fwouts deleted the postcss-loader-upgrade branch March 3, 2022 09:43
@patak-cat
Copy link
Copy Markdown
Member

patak-cat commented Mar 10, 2022

Netlify is failing to build the docs since this PR was merged. Error

9:39:12 PM: /opt/build/repo/node_modules/.pnpm/[email protected][email protected]/node_modules/terser/dist/bundle.min.js → dist...
9:39:13 PM: created dist in 1s
9:39:14 PM: > [email protected] build-types
9:39:14 PM: > run-s build-temp-types patch-types roll-types
9:39:14 PM: > [email protected] build-temp-types
9:39:14 PM: > tsc --emitDeclarationOnly --outDir temp/node -p src/node
9:39:23 PM: ../../node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-load-config/src/index.d.ts(3,23): error TS2307: Cannot find module 'postcss/lib/processor' or its corresponding type declarations.
9:39:23 PM: ../../node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-load-config/src/index.d.ts(4,53): error TS2307: Cannot find module 'postcss' or its corresponding type declarations.
9:39:23 PM: ERROR: "build-temp-types" exited with 2.
9:39:23 PM: ERROR: "build-types" exited with 1.
9:39:23 PM: ERROR: "build-vite" exited with 1.
9:39:23 PM: ​
9:39:23 PM: ────────────────────────────────────────────────────────────────
9:39:23 PM:   "build.command" failed                                        
9:39:23 PM: ────────────────────────────────────────────────────────────────
9:39:23 PM: ​
9:39:23 PM:   Error message
9:39:23 PM:   Command failed with exit code 1: npx pnpm i --store=node_modules/.pnpm-store && npm run ci-docs
9:39:23 PM: ​
9:39:23 PM:   Error location
9:39:23 PM:   In build.command from netlify.toml:
9:39:23 PM:   npx pnpm i --store=node_modules/.pnpm-store && npm run ci-docs

9:38:17 PM: Python version set to 3.7
9:38:20 PM: v16.14.0 is already installed.
9:38:20 PM: Now using node v16.14.0 (npm v8.3.1)

Any ideas? I can't reproduce the issue locally.

@fwouts
Copy link
Copy Markdown
Contributor Author

fwouts commented Mar 10, 2022

Netlify is failing to build the docs since this PR was merged. Error

9:39:12 PM: /opt/build/repo/node_modules/.pnpm/[email protected][email protected]/node_modules/terser/dist/bundle.min.js → dist...
9:39:13 PM: created dist in 1s
9:39:14 PM: > [email protected] build-types
9:39:14 PM: > run-s build-temp-types patch-types roll-types
9:39:14 PM: > [email protected] build-temp-types
9:39:14 PM: > tsc --emitDeclarationOnly --outDir temp/node -p src/node
9:39:23 PM: ../../node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-load-config/src/index.d.ts(3,23): error TS2307: Cannot find module 'postcss/lib/processor' or its corresponding type declarations.
9:39:23 PM: ../../node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-load-config/src/index.d.ts(4,53): error TS2307: Cannot find module 'postcss' or its corresponding type declarations.
9:39:23 PM: ERROR: "build-temp-types" exited with 2.
9:39:23 PM: ERROR: "build-types" exited with 1.
9:39:23 PM: ERROR: "build-vite" exited with 1.
9:39:23 PM: ​
9:39:23 PM: ────────────────────────────────────────────────────────────────
9:39:23 PM:   "build.command" failed                                        
9:39:23 PM: ────────────────────────────────────────────────────────────────
9:39:23 PM: ​
9:39:23 PM:   Error message
9:39:23 PM:   Command failed with exit code 1: npx pnpm i --store=node_modules/.pnpm-store && npm run ci-docs
9:39:23 PM: ​
9:39:23 PM:   Error location
9:39:23 PM:   In build.command from netlify.toml:
9:39:23 PM:   npx pnpm i --store=node_modules/.pnpm-store && npm run ci-docs

9:38:17 PM: Python version set to 3.7
9:38:20 PM: v16.14.0 is already installed.
9:38:20 PM: Now using node v16.14.0 (npm v8.3.1)

Any ideas? I can't reproduce the issue locally.

Sorry about that! I think this may be fixed by adding postcss as an explicit dev dependency in packages/vite.

@fwouts fwouts mentioned this pull request Mar 28, 2022
Merged
9 tasks
@bluwy bluwy mentioned this pull request Mar 29, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat patak-cat approved these changes

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Milestone

2.9

Development

Successfully merging this pull request may close these issues.

PostCSS plugins cannot be resolved when root != cwd

4 participants

@fwouts @Niputi @patak-cat @hpx7