Js-Brecht / gatsby-plugin-pnpm

Provides PNPM compatible module resolvers to Webpack for Gatsby
MIT License
48 stars 6 forks source link

Option for nodeModules path #3

Closed cdfa closed 4 years ago

cdfa commented 4 years ago

Hi, I'm using docz as documentation generator, which uses gatsby under the hood and suffers from the same problems as normal gatsby usage. However, the documentation is built in a subfolder of the project (.docs) instead of the root, so this plugin fails to find the node_modules directory. I was able to fix this by changing const nodeModules = path.join(process.cwd(), 'node_modules'); to const nodeModules = path.join(process.cwd(), '..', 'node_modules');.

Could we make the node_modules path configurable as an option for this plugin?

Js-Brecht commented 4 years ago

That feature was added in PR #2, specifically to address these kinds of issues. You should be able to include that directory as an additional option, kind of like this:

// gatsby-config
module.exports = {
    plugins: [
        {
            resolve: `gatsby-plugin-pnpm`,
            options: {
                include: [
                    path.join('..', 'node_modules') // Note: for relative paths like this, it uses process.cwd() automatically
                ]
            }
        }
    ]
}

See also https://github.com/Js-Brecht/gatsby-plugin-pnpm#available-options. One other note about it: it will always order your current working directory's node_modules first, and then layer on any additional include options.

If that doesn't work, then there is a bug somewhere. Please let me know

Js-Brecht commented 4 years ago

If Webpack used Node's module resolution, this wouldn't even be an issue, because Node automatically looks for node_modules while walking up the directory tree 😕

Js-Brecht commented 4 years ago

Also, you will need to use v1.1.1. I had decided to rename resolutions to include before releasing v1.1.0, to avoid any confusion with how resolutions work in other contexts (like with yarn, that sort of thing). Apparently, I missed a vital one 😆

cdfa commented 4 years ago

Ah, my apologies. I hadn't understood the include option could be used for this. The problem seems to be slightly different now: the walkback starts from .docz/node_modules/gatsby, which doesn't exist, because no dependencies are actually installed in the build directory. I also get the following error:

ERROR #11321  PLUGIN

"gatsby-plugin-pnpm" threw an error while running the onCreateWebpackConfig lifecycle:

ENOENT: no such file or directory, lstat '/home/cdfa/Projects/react-turn-reveal/.docz/node_modules/gatsby

Because the script threw an error, the paths are not added to the webpack config. How can this be solved?

Js-Brecht commented 4 years ago

Yeah, since this is a gatsby plugin, it requires gatsby to run. That does, however, point out something I overlooked when resolving the gatsby directory.

So, it's not a sub-project, precisely. It looks like it actually spawns a sub-process, and that may be why it's changing your process.cwd() to a subdirectory. That's a little strange; most package authors are able to do their work without changing your process's current working directory.

Oh well. I'm fine with adding a new option for defining your "root" node modules location; I'll probably call it projectPath (i.e. define the path to the folder that would contain your project's node_modules, and it will find node_modules from there). That's better than allowing it to skip the gatsby resolution, and it will help keep more in line with the pnpm philosophy of limiting your project's module scope.

I'll be able to get that out here pretty quick, and we can do a trial run

cdfa commented 4 years ago

The way it works is strange indeed. I think there is some logic it has to fool, because it also copies over my gatsby-node.js and gatsby-config.js from the project root to the subdirectory and creates a fake package.json to make things work. Anyway, docz seems very complex, so I'm very happy the issue can be resolved here :smiley: .

The solution you have in mind sounds good!

Js-Brecht commented 4 years ago

I noticed that when I glanced over their source, but I didn't think much of it. So what it sounds like they're doing is dumping all of the data they need into a structure that gatsby can understand, moving over your gatsby configurations, and then running it all from that directory.

So, because of this, when using something like this:

path.resolve(process.cwd(), '..');

you won't be able to run Gatsby on your actual project, because it will be looking outside of your project root.

You could, however, do a check to see what directory Gatsby is being run from:

const curDir = process.cwd();
projectPath: path.basename(curDir) === '.docz' ? path.resolve(curDir, '..') : curDir

But it seems foolish to me to require people to base that kind of thing off of an arbitrary string/folder name like that. I think I will also include a strict option, which will default to true. If set to false, then require.resolve() will be used to resolve Gatsby's path, and any package names defined in include. This means that the standard node module resolution will be used, which walks backwards up the tree from your current working directory, looking for node_modules.

cdfa commented 4 years ago

I'm not sure if I completely understand, but luckily, I don't think I have to distinguish between those cases. I don't use Gatsby in the "actual project", so I can just use path.resolve(process.cwd(), '..');.

Might be useful in different projects though :)

Js-Brecht commented 4 years ago

Care to try targeting Js-Brecht/gatsby-plugin-pnpm#release-v1.2.0 as your dependency version?

Js-Brecht commented 4 years ago

docs are here: https://github.com/Js-Brecht/gatsby-plugin-pnpm/tree/feature-add-strict-projectPath

cdfa commented 4 years ago

Using projectPath works for me :smile: . Thanks a lot!

Js-Brecht commented 4 years ago

:rocket: v1.2.2 is live