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(yarn2): install node_modules for node-modules linker #811

Conversation

mlavina
Copy link

@mlavina mlavina commented Jul 23, 2020

Branching off this comment #795 (comment) because I want to use heroku buildpack with yarn 2 and nodeLinker: node-modules

This is a rough draft to through something up. Will convert to a regular draft when it's ready to be reviewed.

@mlavina mlavina force-pushed the mlavina/feat/yarn2/installForNodeModulesLinker branch 3 times, most recently from ab7dfe1 to 9fa61a7 Compare July 23, 2020 15:50
@mlavina mlavina closed this Jul 23, 2020
@mlavina mlavina reopened this Jul 23, 2020
@mlavina mlavina force-pushed the mlavina/feat/yarn2/installForNodeModulesLinker branch 7 times, most recently from e8d8d77 to d7ba67f Compare July 27, 2020 18:41
@mlavina mlavina marked this pull request as ready for review July 27, 2020 19:07
@mlavina mlavina requested a review from a team as a code owner July 27, 2020 19:07
@mlavina
Copy link
Author

mlavina commented Jul 27, 2020

This should be good to go. This works for my unit tests, but it suddenly started breaking after updating to the latest. I think it's related to #815 but I am unsure.

@danielleadams can you take a look when you have a chance since I based this off your PR? Thank you.

@mlavina mlavina force-pushed the mlavina/feat/yarn2/installForNodeModulesLinker branch from d7ba67f to be74a82 Compare July 29, 2020 12:40
@mlavina
Copy link
Author

mlavina commented Jul 30, 2020

@edmorley @danielleadams just wanted to conform there is nothing else I need to do with this PR. I didn't find a contributing doc.

I added a new unit test and updated Changelog. I tested this locally with my own project as well and it worked.

@danielleadams
Copy link
Contributor

Thanks @mlavina! Sorry, this week has been a little crazy, but thanks for the PR. I'll try to take a look by EOD

@mlavina
Copy link
Author

mlavina commented Jul 30, 2020

@danielleadams thank you. Sorry for adding extra work to your plate, haha.

@danielleadams
Copy link
Contributor

danielleadams commented Jul 30, 2020

@mlavina this looks good, but #819 will change the behavior of the buildpack with Yarn 2 (yarn install will be run by default with the Yarn 2 flags). Once that PR goes in, the buildpack will be set up to more easily use node modules. (Tests are currently failing, but that's what it will look like)

@mlavina
Copy link
Author

mlavina commented Aug 5, 2020

Should I close this PR in favor of #819 #821 i don't know if this PR does anything different since we are going to run yarn install anyway

@danielleadams
Copy link
Contributor

@mlavina yeah, I think so. Even so, thanks for the work here and feel free to continue the conversation on #821

@mlavina
Copy link
Author

mlavina commented Aug 7, 2020

closing in favor of #821

@mlavina mlavina closed this Aug 7, 2020
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.

2 participants