Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(run): Include the workspace root .bin in env path #4848

Merged
merged 11 commits into from
Nov 20, 2017

Conversation

rally25rs
Copy link
Contributor

@rally25rs rally25rs commented Nov 2, 2017

Summary

Partial resolution for #4543.

Previously, when running yarn run the env PATH would be set to look in node_modules/.bin, however, in workspaces the root workspace .bin path was not being included.

This PR adds the workspace root
node_modules/.bin path after the individual package's path.

This is generally needed because #4730 ensures bin links in a workspace will be at the workspace root. With this PR, you can now yarn run commands in an individual package again.

Test plan

Manually tested by adding a script that runs echo $PATH

When running `yarn run` the env PATH would be set to look in node_modules/.bin, however in
workspaces the root workspace .bin path was not being included. This PR adds the workspace root
node_modules/.bin path after the individual package's path.

Partial resolution for yarnpkg#4543
@buildsize
Copy link

buildsize bot commented Nov 2, 2017

This change will increase the build size from 10.24 MB to 10.24 MB, an increase of 213 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 885.44 KB 885.38 KB -60 bytes (0%)
yarn-[version].js 3.85 MB 3.85 MB 139 bytes (0%)
yarn-legacy-[version].js 4 MB 4 MB 139 bytes (0%)
yarn-v[version].tar.gz 890.08 KB 890.2 KB 129 bytes (0%)
yarn_[version]all.deb 667.31 KB 667.18 KB -134 bytes (0%)

@BYK BYK self-requested a review November 6, 2017 12:30
@BYK BYK self-assigned this Nov 6, 2017
@BYK
Copy link
Member

BYK commented Nov 6, 2017

This is awesome! Let's add at least one test, merge from master so we get the latest CI upgrades and I'm game!

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending back for tests :)

Copy link
Member

@kaylie-alexa kaylie-alexa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@rally25rs
Copy link
Contributor Author

rally25rs commented Nov 13, 2017

@BYK I finally got around to fixing this test. All ✅ now 👍

const logData = logEntry ? logEntry.data.toString() : '{}';
const parsed = logEntry ? JSON.parse(logData) : {};
let envPaths = [];
if (parsed.PATH) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use path.delimiter instead. I'll go ahead and fix this myself and then merge tho :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, I didn't even know that was a thing. Thanks!

envPaths = parsed.Path.split(';');
}

expect(envPaths).toContain(path.join(config.cwd, 'node_modules/.bin'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this to fail on Windows since the path separator should be \ there. Weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC path.join() converts it auto-magically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although... why didn't I do path.join(config.cwd, 'node_modules', '.bin')? 😕 ...I'm an idiot 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice.

@BYK BYK merged commit 1ea7ef8 into yarnpkg:master Nov 20, 2017
@iansu
Copy link
Contributor

iansu commented Dec 18, 2017

Any idea when this is going to be released?

@jonaskello
Copy link

jonaskello commented Feb 7, 2018

I cannot get this to work. Is it included in 1.3.2?

UPDATE: I checked and it seems it is not in 1.3.2 but it is in 1.4.1.

@simonbuchan
Copy link

I think there was some talk about putting the release tags a merged PR is included by, like they are in commits, but for now you can check in the footer of the commit message on the page for the merge commit, which is linked in the merge message above.

@rally25rs rally25rs deleted the workspace-run-path branch February 16, 2018 23:24
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Sep 13, 2018
Before 1.4.0 bin commands could not be found when executed from
a workspace. See yarnpkg/yarn#4848
oliviertassinari pushed a commit to mui/material-ui that referenced this pull request Sep 13, 2018
* [core] Set required yarn version

Before 1.4.0 bin commands could not be found when executed from
a workspace. See yarnpkg/yarn#4848

* Revert "[core] Set required yarn version"

This reverts commit e6260e3.

Netlify uses 1.3.2 which has the same issue. This also limits
users not just contributors.

* [core] Add note about build prerequisites

* Update CONTRIBUTING.md
marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018
* [core] Set required yarn version

Before 1.4.0 bin commands could not be found when executed from
a workspace. See yarnpkg/yarn#4848

* Revert "[core] Set required yarn version"

This reverts commit e6260e3.

Netlify uses 1.3.2 which has the same issue. This also limits
users not just contributors.

* [core] Add note about build prerequisites

* Update CONTRIBUTING.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants