Add Yarn Workspaces Support

jbalsas commented 5 years ago

In a Yarn workspace context, most liferay-npm-scripts will fail to run from within the workspaces.

Using the attached simple project: From root folder

yarn install
yarn workspace Foo run build // works!

From packages/Foo folder

npm run build // fails!

The main problem is that liferay-npm-scripts is the only devDependency on the workspace, so yarn will only create that symlink in the /packages/Foo/node_modules/.bin folder (as expected).

The binaries for this package dependencies will end up in /node_modules/liferay-npm-scripts/node_modules/.bin, which are unreachable by npm-which that will only look in CWD and above.

A possible fix would be to have different npm-which resolutions:

const cwdWhich = require('npm-which')(process.cwd());
const dirnameWhich = require('npm-which')(__dirname);

let babel;

try {
    babel = cwdWhich('babel');
catch () {
    babel = dirnameWhich('babel');
}

Note that scripts will work if ran from the root of the project with the yarn workspace Foo run build command.

Since I've only had limited experience with Yarn, I'm not sure what exactly should be expected... @wincent, any thoughts?

yarn_workspaces.zip

wincent commented 5 years ago

I haven't used npm-which before, but my first instinct here is to not bother trying to use it. I would teach "liferay-npm-scripts" itself to be workspace-aware and do the right thing when inside a workspace, falling back to existing behavior in a non-workspace context.

Two things:

If we have this stuff in a monorepo then there are ways we can force stuff to always be run from the top. I think the best way would be to bake that into liferay-npm-scripts (which could effectively make it transparent), but you could do stuff as crude as having a template that made default scripts of the form echo "Please run from top-level"; false etc.
I think you can nest workspaces (something we might want to do in the liferay-portal repo, for example, where we could have a repo like Clay being a monorepo/workspace and have that nested inside a larger workspace corresponding to the Portal), but from what I understand there can be some rough edges with this. Nested workspaces are currently listed on the roadmap but I suspect (haven't verified) that there is some version of this on master already?

One minor thing about your example: "liferay-npm-scripts" should be in devDependencies, I think, although I don't think that will change the behavior you're talking about in this issue.

Disclaimer: writing this at 5 AM at airport.

bryceosterhaus commented 5 years ago

I dug into yarn workspaces a bit and didn't necessarily find the answers I was looking for. I tried removing npm-which and it seems to work so far. Not sure how exactly we want to use this in the context of a portal workspace though.

One issue I ran into was when trying to run npm run build inside the Foo package I get this error, might be from an error in npm-bundler though. I haven't been able to figure out why this is failing.

internal/modules/cjs/loader.js:613
    throw err;
    ^

Error: Cannot find module '../lib/config'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:611:15)
    at Function.Module._load (internal/modules/cjs/loader.js:537:25)
    at Module.require (internal/modules/cjs/loader.js:665:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/bryceosterhaus/liferay/repos/liferay-npm-build-tools/packages/liferay-npm-bundler/bin/liferay-npm-bundler.js:2:14)
    at Module._compile (internal/modules/cjs/loader.js:736:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:747:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:568:12)
    at Function.Module._load (internal/modules/cjs/loader.js:560:3)

This does not fail when running yarn run build, but I wasn't sure what we are expecting from developers. Are we expecting yarn to be used entirely?

here is the PR removing npm-which https://github.com/bryceosterhaus/liferay-npm-tools/pull/10

jbalsas commented 5 years ago

Thanks @bryceosterhaus! I'll be trying this out with https://github.com/jbalsas/liferay-portal/pull/1646...

As a first step, the expectation is that developers could still run their tests or other scripts individually npm run build, npm test, npm run start... if need be, we could use yarn instead.

The only difference would be that with Node.js, npm is already in the global path, so that's the only thing they need to download right now. If we had to go with Yarn, that'd add at least an additional npm i -g yarn. Not a huge deal, but a deal, nonetheless 🤷‍♂️

liferay / liferay-npm-tools

Add Yarn Workspaces Support #9