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

chore(deps): update source-map@^0.7.3 to 0.7.4 #6560

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Nov 11, 2022

Refs: #6489

Description

The source-map@0.7.4 update is important for enabling Node.js v18 compatibility. Older versions use a typeof fetch === 'function' check to enable browser-specific behavior. With this release however, the browser check is moved to whether window is defined and matches this value at check time. Node.js 18 has a fetch function, so older source-map versions incorrectly flag it as 'browser'. None of Node.js versions have a this === window situation going on.

This is a change on the path to Node.js 18 compatibility route, in anticipation of #6489 offering official support down the line.

Refs: mozilla/source-map@48f90d5

Security Considerations

Assuming existing dependency ranges are assumed to be secure, this is not applicable.

Documentation Considerations

N/A. No changes to user are expected, as the internal update is a semver patch.

Testing Considerations

Existing regression testing cases apply here, as no new functionality is being introduced.

@sigv
Copy link
Contributor Author

sigv commented Nov 11, 2022

I am not sure about the existing release strategy and what gets merged where, however this can be cherry-picked into release-pismo for providing a new pismo build that should be Node.js 18 compatible.

I validated the packages/cosmic-swingset testsuite, as that is what I am most interested in. However I will also run the full testsuite, as it sounds likely to me that the testcases can be enabled for Node.js 18.x on the repository after this change.

@sigv
Copy link
Contributor Author

sigv commented Nov 13, 2022

Deployed a fresh Ubuntu 22.04 (Jammy) image in Azure running on D16s_v5 (16 vCPUs, 64G RAM) for the test suite.

curl -fsSL https://deb.nodesource.com/gpgkey/nodesource.gpg.key | gpg --dearmor | sudo tee /etc/apt/keyrings/nodesource.gpg >/dev/null
echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_18.x $(lsb_release -s -c) main" | sudo tee /etc/apt/sources.list.d/nodesource.list >/dev/null

sudo apt update
sudo apt --yes dist-upgrade
sudo apt --yes install chromium-browser gcc g++ jq make nodejs perl

sudo corepack enable yarn

sudo wget -O /usr/local/go1.19.3.linux-amd64.tar.gz https://go.dev/dl/go1.19.3.linux-amd64.tar.gz
sudo tar -C /usr/local -xf /usr/local/go1.19.3.linux-amd64.tar.gz
eval $(echo 'export PATH=$PATH:/usr/local/go/bin' | sudo tee /etc/profile.d/golang.sh)

go version
# go version go1.19.3 linux/amd64
node --version
# v18.12.1
yarn --version
# 1.22.19

git clone -b maint/source-map-0.7.4 https://github.com/sigv/agoric-sdk.git
cd agoric-sdk

yarn install
yarn build
yarn test
# all tests reported as passed/skipped/todo (no failures)

Would say LGTM.

@sigv sigv mentioned this pull request Nov 13, 2022
@mhofman
Copy link
Member

mhofman commented Nov 14, 2022

Thanks for the contribution, and for providing result of local tests. We definitely need to setup CI for node 18 to double check that this is the only required change.

I am not sure about the existing release strategy and what gets merged where, however this can be cherry-picked into release-pismo for providing a new pismo build that should be Node.js 18 compatible.

As I mentioned in #6489 (comment), we do not plan on picking up such changes into the release-pismo branch. While it seems that the changes from 0.7.3 to 0.7.4 of source-map are limited to this bug fix and some wasm option we probably don't trigger, and we currently don't leverage source-map, it is technically a transitive dependency of endo's bundle-source, and I would be way too worried that the bundles created by the new version may differ from the bundles created by the previous version, which would cause consensus failures.

@mhofman mhofman added the automerge:squash Automatically squash merge label Nov 14, 2022
@mhofman
Copy link
Member

mhofman commented Nov 14, 2022

@sigv apparently Mergify cannot push to your branch. Please rebase on or merge from master manually, or allow edits from maintainers to your branch.

The `source-map@0.7.4` update is important for enabling Node.js v18
compatibility. Older versions use a `typeof fetch === 'function'`
check to enable browser-specific behavior. With this release however,
the browser check is moved to whether `window` is defined and matches
`this` value at check time. Node.js 18 has a `fetch` function, so
older `source-map` versions incorrectly flag it as 'browser'. None
of Node.js versions have a `this === window` situation going on.

This is a change on the path to Node.js 18 compatibility route,
in anticipation of Agoric#6489 offering official support down the line.
@sigv
Copy link
Contributor Author

sigv commented Nov 14, 2022

Pushed a rebase via Web UI.

Doing so results in approval required for pipeline execution: First-time contributors need a maintainer to approve running workflows.

@sigv
Copy link
Contributor Author

sigv commented Nov 14, 2022

I would be way too worried that the bundles created by the new version may differ from the bundles created by the previous version, which would cause consensus failures.

Understandable — I was thinking this could land together with the state-sync fix, if that is pushed out. However if you believe the risk is too high of this being a breaking change, that makes sense.

Thank you for the comment!

@mhofman
Copy link
Member

mhofman commented Nov 14, 2022

Doing so results in approval required for pipeline execution

Approved and now running.

this could land together with the state-sync fix, if that is pushed out

The current plan for state sync is that it'll only be part of the next major version. That said, with my limited understanding of cosmos node operations, I suggested a workaround here in the meantime.

@sigv
Copy link
Contributor Author

sigv commented Nov 14, 2022

Chain deployment test / deployment-test failed on go mod download with zip: not a valid zip file 🤔

@mhofman
Copy link
Member

mhofman commented Nov 14, 2022

It can't be related so I'll assume it's a transient network connection issue and re-ran the job.

@sigv
Copy link
Contributor Author

sigv commented Nov 14, 2022

@mhofman, can you please reschedule the mergify: Queue: Embarked in merge train? It seems it picked up the previous deployment test failure, but does not get triggered upon a successful re-run.

@mhofman
Copy link
Member

mhofman commented Nov 14, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Nov 14, 2022

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 4425dc4 into Agoric:master Nov 14, 2022
@sigv sigv deleted the maint/source-map-0.7.4 branch November 14, 2022 14:18
@sigv
Copy link
Contributor Author

sigv commented Nov 14, 2022

Thank you 🎉

mhofman pushed a commit that referenced this pull request Jun 6, 2023
The `source-map@0.7.4` update is important for enabling Node.js v18
compatibility. Older versions use a `typeof fetch === 'function'`
check to enable browser-specific behavior. With this release however,
the browser check is moved to whether `window` is defined and matches
`this` value at check time. Node.js 18 has a `fetch` function, so
older `source-map` versions incorrectly flag it as 'browser'. None
of Node.js versions have a `this === window` situation going on.

This is a change on the path to Node.js 18 compatibility route,
in anticipation of #6489 offering official support down the line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants