fix: load PostCSS plugins based on the current file path instead of process.cwd()

fwouts commented 2 years ago

`Notable Changes`

Currently, postcss-load-config can only work when the desired PostCSS plugins can be loaded from process.cwd(). It does not work, for example, when postcss-load-config is used to load a PostCSS config file from a different directory, and that PostCSS config file uses a plugin that isn't available in the current working directory.

Until https://github.com/postcss/postcss-load-config/pull/227, a workaround was to call process.chdir() before invoking postcss-load-config, as it would appropriately update the require path and find the PostCSS plugins. This workaround was discussed in https://github.com/vitejs/vite/issues/4000 for example.

Since https://github.com/postcss/postcss-load-config/pull/227, process.cwd() is invoked once at module initialisation time, which is a subtly different behaviour from import-cwd which invoked process.cwd() repeatedly on every require (see https://github.com/sindresorhus/import-cwd/blob/8e204cbf8b4fc0743e4ebeebd0acfbbd17fee87b/index.js#L4). This broke the workaround of calling process.chdir() since it would have no effect on later invocations of postcss-load-config.

A better approach was suggested by @43081j in https://github.com/vitejs/vite/issues/4000#issuecomment-1005655018, which relies on using the PostCSS config file as the search path for loading PostCSS plugins, instead of process.cwd(). This seems sensible, and all tests pass, but I may be missing a subtle requirement here.

If this behaviour isn't desirable, we could at least restore the old workaround by replacing:

const req = (createRequire || createRequireFromPath)(path.resolve(process.cwd(), '_'))

with:

const req = moduleId => (createRequire || createRequireFromPath)(path.resolve(process.cwd(), '_'))(moduleId)

(effectively deferring the call to process.cwd())

`Commit Message Summary (CHANGELOG)`

fix: load plugins from postcss config path instead of current working directory

`Type`

[ ] CI
[x] Fix
[ ] Perf
[ ] Docs
[ ] Test
[ ] Chore
[ ] Style
[ ] Build
[ ] Feature
[x] Refactor

`SemVer`

[x] Fix (:label: Patch)
[ ] Feature (:label: Minor)
[ ] Breaking Change (:label: Major)

`Issues`

Fixes https://github.com/vitejs/vite/issues/4000

`Checklist`

[x] 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)
[x] Any dependent changes are merged and published in downstream modules

ai commented 2 years ago

I like your way to look file from config path

ai commented 2 years ago

Released in 3.1.2

fwouts commented 2 years ago

Wow, that was fast :) Thank you!

mojavelinux commented 2 years ago

This change broke the loading of plugins defined as follows:

module.exports = (ctx) => ({
  plugins: {
    './lib/postcss-minify-selectors.js': true,
    './lib/postcss-rule-per-line.js': true,
  }
})

I'm running postcss from the directory where the config file is located and the lib folder is in that directory. The message I get is:

Error: Loading PostCSS Plugin failed: Cannot find module './lib/postcss-minify-selectors.js'
Require stack:
- /path/to/project/postcss.config.js/_

Based on what I'm observing, I don't think this change should have been made in a patch release. This seems to a breaking change, which should have warranted a major release. I'm also curious to know why it can't find the file that seems to be in the right place, but there are ways I can work around it.

module.exports = (ctx) => ({
  plugins: {
    [require.resolve('./lib/postcss-minify-selectors.js')]: true,
    [require.resolve('./lib/postcss-rule-per-line.js')]: true,
  }
})

43081j commented 2 years ago

It shouldn't really be a breaking change since it didn't intentionally break something, you probably just found a bug.

Could you possibly recreate it in a GH repo? so it can be debugged

mojavelinux commented 2 years ago

Changing the mechanism for how modules and scripts are required is a breaking change. The issue clearly states that the behavior is being changed. If you don't want to adhere to semver, that's your choice. But you can't argue that this is a patch fix.

It's already reproducible with an existing GH repo. You can see the problem in this build: https://github.com/asciidoctor/asciidoctor/runs/5150765869?check_suite_focus=true build from this commit hash: https://github.com/asciidoctor/asciidoctor/tree/1cfe82949c2634e4efa145c23be3014711575708/src/stylesheets

mojavelinux commented 2 years ago

This line:

const req = (moduleId, file) => (createRequire || createRequireFromPath)(path.resolve(process.cwd(), '_'))(moduleId)

changed to this line:

const req = (moduleId, file) => (createRequire || createRequireFromPath)(path.resolve(file, '_'))(moduleId)

That's not a patch fix. And the description of this PR starts with "Notable Changes". Nothing about a patch release should be notable.

mojavelinux commented 2 years ago

The difference is that the createRequire is called with a path that is one level deeper than before. As a result, Node.js does not allow a relative path to be resolved. It will find an installed module because it continues to look upwards until it finds it. But a relative path to a standalone JS file now has to be changed to an absolute path.

fwouts commented 2 years ago

@mojavelinux I tend to agree that a major version is warranted if the official API has changed.

I'm not very familiar with this project, so I'm not sure whether or not the relative path syntax you're using was ever part of the official API. I don't see such examples in the README or in tests, so I'm not sure.

Given the likelihood that other people are affected by this change of behaviour, documented or not, perhaps it would make sense to revert this PR, release another patch including the revert, then revert the revert and release a major version?

In addition, we may want to add your particular example to the README, using require.resolve() like you showed above, to make sure it never breaks in the future, no matter what base path require works from?

That way we're all happy :)

43081j commented 2 years ago

Changing the mechanism for how modules and scripts are required is a breaking change

It is, and that isn't what we did here. We tried to fix the mechanism as it contained a bug.

Point is, if it worked bug-free, you would've never noticed because it wouldn't have been "breaking". The fact you (probably) discovered a bug makes it seem breaking.

Anyway, i'll see if i can take a look unless andrey beats me to it

mojavelinux commented 2 years ago

@fwouts Sounds very reasonable to me.

I had followed a tutorial somewhere that explained how to set up a custom PostCSS rule inside the project. Unfortunately, I can't recall the location of that tutorial. But that's how I knew to use the relative path.

I'm happy to contribute a change to the documentation to show that you can use a standalone JS file, but that it must be specified as an absolute path.

As I stated above, I understand what's now required. I posted to make you aware that it broke my build and it has the potential to break others. And, naturally, people don't expect a patch release to break things. That's why I brought it up.

43081j commented 2 years ago

@ai FYI the problem here is that we had _ as a placeholder so that path.resolve(dir, '_') would become a path to a file inside dir (as thats what createRequire wants).

since we don't pass a directory anymore, but a file, this results in some-file.js/_ which will lead node to thinking we want to require things from a directory called some-file.js.

if we always pass a file path to req, we can drop the path.resolve i think:

const require = create(rootFile);

then if we ever want to resolve from a directory, do the resolve at the call-site rather than inside req.

ai commented 2 years ago

Yeap, please send PR.

ai commented 2 years ago

@43081j’s fix was released in 3.1.3.

@mojavelinux old behaviour was a bug (and was not officially documented). require’s behavior is to use current file as ..

Of course, we can be extra-cautiou and release notable fixes as major changes, but PostCSS ecosystem is very slow for updates. It could take a years to update all tools using postcss-load-config. I decide that unexpcted changes will affect not big amount of people and it worth to do it in this way.

We can document new behaviour and make in official. Please, send PR.

mojavelinux commented 2 years ago

Thanks for the fix. I'll proceed with a PR to the docs.

postcss / postcss-load-config