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(yarn): migrate from v1.2.17 to v3.6.0 #1475

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

petermetz
Copy link
Member

@petermetz petermetz commented Oct 22, 2021

We hope to solve issues with this that were plaguing contributors due to
bugs in the much older Yarn 1.x versions that we had to use because Yarn V3
had it's own set of problems when it came to linking the .bin folders for
child packages in a monorepo.

  1. We've moved from hoisting by default to NOT hoisting by default for
    all packages which results in slightly slower installs for the dependencies
    when setting up the project build but it does have the beneficial effect
    of much easier fixes for compiler errors because each package can be
    provided with their own version of whatever dependencies they need.
  2. The "pre" and "post" npm scripts are no longer supported by Yarn
    which was an annoying breaking change so we had to restructure some of the
    scripts which had pre and post scripts of their own to get equal behavior.
  3. Upgraded to Yarn v3.6.0
  4. packages/cactus-api-client/src/main/typescript/socketio-api-client.ts
    has a line where the Typescript compiler has been disabled and an issue
    opened for the same: build(api-client): fix tsc errors of @types/jsonwebtoken verify call #2523
  5. Added several missing dependencies to the individual package.json
    files of the packages. These were resolving fine earlier because of hoisting
    but the problem of undeclared dependencies went undetected because of this
    convenience feature of the NodeJS module resolution algorithm. Now,
    instead of having to catch these problems in production, we can fix them
    up-front at development time which is great because there were dozens of
    them!
  6. Migrated from import to import type in a few places in the code
    where it was applicable and useful: when you only import a dependency
    via import type then it's safe to move it to "devDependencies" because
    at runtime the import type syntax is completely omitted by the compiler.
  7. Refactored the importing of "internal-ip" in a couple of test cases
    because the newer version does not export v4 and v6 but instead
    internalIpV4 which meant that the >=7.0.0 versions needed downgrading.
  8. Renamed 2 environment variables that are used by ci.yaml to control
    the steps taken by ci.sh because they had a YARN_ prefix in their
    names that Yarn v3 appears to be automatically parsing and assuming to
    be configuration for itself - which then leads to crashes because it
    does not recognize the custom variable name that is not meant for it.
    The variables renamed are: TOOLS_VALIDATE_BUNDLE_NAMES_DISABLED and
    CUSTOM_CHECKS_DISABLED
  9. The configure script now runs the init-registries sub-script via npm
    instead of yarn because yarn is unable to run scripts prior to having ran
    a yarn install on a fresh clone (which was making the configure script
    bail out on the CI)
  10. Configured yarn NOT to rebuild most dependencies that have native
    code in them. The exception from under this is the gRPC and related
    dependencies whose native code extensions are being used by the test
    cases and so we do need them for real.
  11. Updated the artillery binary path to be relatively qualified instead
    of just the binary file's name in the test case that does the API server
    benchmarking in
    packages/cactus-cmd-api-server/src/test/typescript/benchmark/artillery-api-benchmark.test.ts
    so now the binary's path is specified explicitly as
    ./packages/cactus-cmd-api-server/node_modules/artillery/bin/artillery
    which is the new path on account of us turning off t he hoisting of the
    dependencies entirely for the project (e.g. the binary is no longer in
    the rood node_modules directory)
  12. Explicitly disabled the build scripts of a long list of dependencies
    in the root package.json file because they were greatly and needlessly
    slowing down the build (we know that we don't need their build scripts
    because it was working well with yarn v1 without them)
  13. Updated the mapping for the foundry/forge dependencies because now
    that we don't have hoisting enabled by default for any of the packages
    the binaries end up in the package's own node_modules instead of the root's.
  14. Refactored the ci.sh script to not do a lot of the resource intensive
    diagnostics and the disk space optimization is also disabled by default.
    These things used to be very important when we were running a monolithic
    CI job for everything but not so much now that we have a separate job for
    every single package. This brings down the ci.sh execution time a few
    minutes at least which adds up to a lot because it is executed 40+ times
    for each CI run.
  15. In general the important change here (apart from the yarn v3 upgrade)
    is the disablement of the hoisting of dependencies because this makes the
    build much more reliable and harder to mess up with missing dependencies.
  16. Had to upgrade protoc-gen-ts to v0.8.6 which then needed typescript
    v4.9.x or newer which then forced me to fix a few new compiler errors
    (which seemed like legit bugs so it's a net positive but it's yet
    another big diff causing change)
  17. Had to fix a few webpack import issues related to backend dependencies
    being used on the front-end - this only affects the example code/GUI
  18. Upgraded the eslint typescript plugin and parser dependencies because
    the linter otherwise couldn't handle the code with the new version of the
    typescript compiler (v4.9.x). Also had to set the ban on explicit anys
    to a warn level down from an error because we have hundreds of them in
    the codebase.
    There's one linter error left that I won't fix here because there is
    already another PR for it.

Fixes #1142

Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com

@petermetz petermetz self-assigned this Oct 22, 2021
@petermetz petermetz force-pushed the deps-yarn-v2 branch 2 times, most recently from a163b06 to 67c8084 Compare October 23, 2021 06:49
@petermetz petermetz added dependencies Pull requests that update a dependency file Developer_Experience enhancement New feature or request labels Nov 16, 2021
@petermetz petermetz force-pushed the deps-yarn-v2 branch 5 times, most recently from 20beaca to 8e642fd Compare November 16, 2021 03:34
@petermetz petermetz changed the title build(yarn): migrate from V1 to V3 build(yarn): migrate from v1.2.17 to v3.6.0 Jul 7, 2023
@petermetz petermetz force-pushed the deps-yarn-v2 branch 15 times, most recently from 7f525e1 to e24a7c7 Compare July 13, 2023 21:11
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

@petermetz petermetz force-pushed the deps-yarn-v2 branch 6 times, most recently from 9d41445 to 2a3d25b Compare July 20, 2023 07:10
@sandeepnRES
Copy link
Member

sandeepnRES commented Jul 20, 2023

@petermetz can you change one file which should make all the weaver CI tests pass:
edit weaver/samples/fabric/fabric-cli/makefile, remove line 3 and 11.
I'd had done it but I don't know how can I make commits to this PR.

@petermetz petermetz force-pushed the deps-yarn-v2 branch 2 times, most recently from 7843a07 to b48d810 Compare July 20, 2023 22:18
We hope to solve issues with this that were plaguing contributors due to
bugs in the much older Yarn 1.x versions that we had to use because Yarn V3
had it's own set of problems when it came to linking the .bin folders for
child packages in a monorepo.

1. We've moved from hoisting by default to NOT hoisting by default for
all packages which results in slightly slower installs for the dependencies
when setting up the project build but it does have the beneficial effect
of much easier fixes for compiler errors because each package can be
provided with their own version of whatever dependencies they need.
2. The "pre" and "post" npm scripts are no longer supported by Yarn
which was an annoying breaking change so we had to restructure some of the
scripts which had pre and post scripts of their own to get equal behavior.
3. Upgraded to Yarn v3.6.0
4. packages/cactus-api-client/src/main/typescript/socketio-api-client.ts
has a line where the Typescript compiler has been disabled and an issue
opened for the same: hyperledger#2523
5. Added several missing dependencies to the individual package.json
files of the packages. These were resolving fine earlier because of hoisting
but the problem of undeclared dependencies went undetected because of this
convenience feature of the NodeJS module resolution algorithm. Now,
instead of having to catch these problems in production, we can fix them
up-front at development time which is great because there were dozens of
them!
6. Migrated from `import` to `import type` in a few places in the code
where it was applicable and useful: when you only import a dependency
via `import type` then it's safe to move it to "devDependencies" because
at runtime the `import type` syntax is completely omitted by the compiler.
7. Refactored the importing of "internal-ip" in a couple of test cases
because the newer version does not export `v4` and `v6` but instead
`internalIpV4` which meant that the >=7.0.0 versions needed downgrading.
8. Renamed 2 environment variables that are used by ci.yaml to control
the steps taken by ci.sh because they had a `YARN_` prefix in their
names that Yarn v3 appears to be automatically parsing and assuming to
be configuration for itself - which then leads to crashes because it
does not recognize the custom variable name that is not meant for it.
The variables renamed are: `TOOLS_VALIDATE_BUNDLE_NAMES_DISABLED` and
`CUSTOM_CHECKS_DISABLED`
9. The configure script now runs the init-registries sub-script via npm
instead of yarn because yarn is unable to run scripts prior to having ran
a yarn install on a fresh clone (which was making the configure script
bail out on the CI)
10. Configured yarn NOT to rebuild most dependencies that have native
code in them. The exception from under this is the gRPC and related
dependencies whose native code extensions are being used by the test
cases and so we do need them for real.
11. Updated the artillery binary path to be relatively qualified instead
of just the binary file's name in the test case that does the API server
benchmarking in
packages/cactus-cmd-api-server/src/test/typescript/benchmark/artillery-api-benchmark.test.ts
so now the binary's path is specified explicitly as
./packages/cactus-cmd-api-server/node_modules/artillery/bin/artillery
which is the new path on account of us turning off t he hoisting of the
dependencies entirely for the project (e.g. the binary is no longer in
the rood node_modules directory)
12. Explicitly disabled the build scripts of a long list of dependencies
in the root package.json file because they were greatly and needlessly
slowing down the build (we know that we don't need their build scripts
because it was working well with yarn v1 without them)
13. Updated the mapping for the foundry/forge dependencies because now
that we don't have hoisting enabled by default for any of the packages
the binaries end up in the package's own node_modules instead of the root's.
14. Refactored the ci.sh script to not do a lot of the resource intensive
diagnostics and the disk space optimization is also disabled by default.
These things used to be very important when we were running a monolithic
CI job for everything but not so much now that we have a separate job for
every single package. This brings down the ci.sh execution time a few
minutes at least which adds up to a lot because it is executed 40+ times
for each CI run.
15. In general the important change here (apart from the yarn v3 upgrade)
is the disablement of the hoisting of dependencies because this makes the
build much more reliable and harder to mess up with missing dependencies.
16. Had to upgrade protoc-gen-ts to v0.8.6 which then needed typescript
v4.9.x or newer which then forced me to fix a few new compiler errors
(which seemed like legit bugs so it's a net positive but it's yet
another big diff causing change)
17. Had to fix a few webpack import issues related to backend dependencies
being used on the front-end - this only affects the example code/GUI
18. Upgraded the eslint typescript plugin and parser dependencies because
the linter otherwise couldn't handle the code with the new version of the
typescript compiler (v4.9.x). Also had to set the ban on explicit anys
to a warn level down from an error because we have hundreds of them in
the codebase.
There's one linter error left that I won't fix here because there is
already another PR for it.

Fixes hyperledger#1142

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
Copy link
Member Author

@petermetz can you change one file which should make all the weaver CI tests pass:
edit weaver/samples/fabric/fabric-cli/makefile, remove line 3 and 11.
I'd had done it but I don't know how can I make commits to this PR.

@sandeepnRES Thank you so much for this! I was about to ask about it! :-)

auto-merge was automatically disabled July 21, 2023 01:29

Rebase failed

@petermetz petermetz merged commit 088da9f into hyperledger:main Jul 21, 2023
106 of 117 checks passed
@petermetz petermetz deleted the deps-yarn-v2 branch July 21, 2023 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Developer_Experience enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build(yarn): migrate from v1.2.17 to v3.6.0
4 participants