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

fix: yarn modern install workspace #13197

Merged
merged 14 commits into from
Mar 27, 2024
Merged

fix: yarn modern install workspace #13197

merged 14 commits into from
Mar 27, 2024

Conversation

0618
Copy link
Contributor

@0618 0618 commented Sep 7, 2023

Description of changes

Issue: Modern Yarn (2,3) doesn't create yarn.lock when running yarn install if it can find a yarn.lock in it's parent directory. So we can see the folloing error:

#13001

image

Fix: Add yarn.lock to category resources directories if there's no yarn.lock

image

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@0618 0618 marked this pull request as ready for review September 7, 2023 23:33
@0618 0618 requested a review from a team as a code owner September 7, 2023 23:33
@Amplifiyer
Copy link
Contributor

Can't we use this new https://yarnpkg.com/configuration/yarnrc#nmHoistingLimits i.e. creating .yarnrc.yml for yarn3 workspaces instead?

Also we should have automated tests for this change.

@Amplifiyer Amplifiyer requested a review from a team September 8, 2023 09:24
@0618
Copy link
Contributor Author

0618 commented Sep 8, 2023

Can't we use this new https://yarnpkg.com/configuration/yarnrc#nmHoistingLimits i.e. creating .yarnrc.yml for yarn3 workspaces instead?

Also we should have automated tests for this change.

That's a good point. Thanks for pointing it out.

However, it only works if we modify the .yarnrc.yml in the root directory. It doesn't work if we add .yarnrc.yml in the resource' directory

However, it work either of the following case:

  1. modify/add .yarnrc.yml in the root directory. OR,
  2. add both .yarnrc.yml and yarn.lock in the resource directory, which means it additionally modify the .yarnrc.yml on top of the current PR

Option 1 is not good because I don't think we want to modify anything outside the amplify/ directory.
Option 2 is not necessary because adding yarn.lock alone is enough for us.

Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

We should add tests.

shared-scripts.sh Outdated Show resolved Hide resolved
Amplifiyer
Amplifiyer previously approved these changes Mar 27, 2024
sobolk
sobolk previously approved these changes Mar 27, 2024
@0618
Copy link
Contributor Author

0618 commented Mar 27, 2024

failed e2e tests are: l_diagnose_hooks_a_mock_api and l_function_3a_init_special_case_http_migration, which are the same as the dev branch. They are not from this PR.

@0618 0618 dismissed stale reviews from sobolk and Amplifiyer via 052d3f7 March 27, 2024 21:14
@0618 0618 merged commit 3ca4aa9 into aws-amplify:dev Mar 27, 2024
5 checks passed
@0618 0618 deleted the fix-yarn-modern branch March 27, 2024 22:20
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.

3 participants