Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Add Yarn Workspaces Support #9

Closed
jbalsas opened this issue Feb 2, 2019 · 3 comments
Closed

Add Yarn Workspaces Support #9

jbalsas opened this issue Feb 2, 2019 · 3 comments

Comments

@jbalsas
Copy link
Contributor

jbalsas commented Feb 2, 2019

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
Copy link
Contributor

wincent commented Feb 3, 2019

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 added a commit that referenced this issue Feb 11, 2019
@bryceosterhaus
Copy link
Member

bryceosterhaus commented Feb 11, 2019

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

bryceosterhaus added a commit that referenced this issue Feb 11, 2019
@jbalsas
Copy link
Contributor Author

jbalsas commented Feb 13, 2019

Thanks @bryceosterhaus! I'll be trying this out with jbalsas/liferay-portal#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 🤷‍♂️

bryceosterhaus added a commit that referenced this issue Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants