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

build: upgrade Yarn to v4 #105

Merged
merged 4 commits into from
Apr 17, 2024
Merged

build: upgrade Yarn to v4 #105

merged 4 commits into from
Apr 17, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 17, 2024

refs: Agoric/agoric-sdk#451

Description

Upgrade from Yarn v1 to v4

Security Considerations

Bulk update to lockfile. I let Yarn do it.

Scaling Considerations

n/a

Documentation Considerations

no

Testing Considerations

CI

@turadg turadg force-pushed the ta/yarn-4 branch 3 times, most recently from ec44d6d to 431eb1f Compare April 17, 2024 20:16
@turadg turadg force-pushed the ta/yarn-4 branch 2 times, most recently from ffd8c59 to 437a839 Compare April 17, 2024 20:44
@turadg turadg requested a review from samsiegart April 17, 2024 20:55
@turadg turadg marked this pull request as ready for review April 17, 2024 20:55
@@ -7,9 +7,7 @@
"main": "dist/index.js",
"scripts": {
"prepack": "tsc --build tsconfig.build.json",
"lint": "run-s --continue-on-error lint:*",
Copy link
Member Author

Choose a reason for hiding this comment

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

run-s doesn't seem to work with yarn 4

@turadg turadg requested a review from dckc April 17, 2024 22:40
Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

Just some non-blocking comments/questions but LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Is yarn lerna obsolete now in the bottom of this file? I'm not sure if yarn 4 has a better way of doing publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can get rid of Lerna eventually, but I don't think it will be very straightforward

@@ -8,12 +8,12 @@
"packages/react-components"
],
"type": "module",
"packageManager": "yarn@1.22.19",
"packageManager": "yarn@4.1.1",
"scripts": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been manually running yarn workspaces run prepack at the top level when building/linking react-components locally. It might be useful to add a prepack script here now that it's more verbose: yarn workspaces foreach --all --topological run prepack

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea. I'll leave for future PR to land this now

@@ -1,6 +1,5 @@
{
"name": "@agoric/react-components",
"private": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember why I added this, I think I was having trouble getting react-components to publish, but it might've been a matter of just adding:

"publishConfig": { "access": "public" }

below. I guess we'll find out, I assume false is the default here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it's the default. This was removed automatically by yarn.

@@ -31,7 +31,6 @@ describe('stringifyRatio', () => {
const ethPrice = harden(
makeRatio(
158724n, // value of 1 eth in cents
// @ts-expect-error fake brand
Copy link
Contributor

Choose a reason for hiding this comment

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

Does yarn 4 have an issue with @ts-expect-error?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into something in merging or rebasing and after having dealt with it several times in in the .ts PR I wanted a quieter solution

@turadg turadg merged commit a6b8c68 into main Apr 17, 2024
1 check passed
@turadg turadg deleted the ta/yarn-4 branch April 17, 2024 23:26
kriskowal added a commit to endojs/endo that referenced this pull request Apr 26, 2024
closes: #2245 
refs: Agoric/agoric-sdk#451
refs: Agoric/ui-kit#105

## Description

Move from Yarn 1 to Yarn 4. Some advantages,

- actively maintained
- [workspace protocol](https://yarnpkg.com/protocol/workspace) (so we
don't have to maintain version numbers in all deps)
- [patch protocol](https://yarnpkg.com/protocol/patch) (so we don't need
patch-package)
- [constraints](https://yarnpkg.com/features/constraints) (e.g. to
enforce layering)
- path to adopt pnpm-style linker (without changing the UI) (see
#1722 )

However this defers workspace protocol until the publishing workflow can
support it.

### Security Considerations

This does a bulk update of `yarn.lock`. It was automated by Yarn 4.

### Scaling Considerations

n/a

### Documentation Considerations

I reviewed `yarn` commands in *.md and I think they're all accurate.

### Testing Considerations

This could interact with the publishing pipeline. @kriskowal may want to
push a draft before we merge. If problems are found, depending on the
severity, we could follow up in a separate PR to land this sooner reduce
merge conflicts.

This was failing on the Windows tests, something about corepack not
taking effect. I don't know whether Windows is officially supported by
Endo. We've since disabled them.
#2243 is the issue restore.


### Compatibility Considerations

Some CLI commands are slightly different. We are adopting it across the
org so we have to adjust sometime.

### Upgrade Considerations

n/a
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