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

Update prebuilt yoga-layout from https://github.com/facebook/yoga #2489

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Jan 3, 2023

What this PR solves / how to test:

This PR attempts to resolve OOM issues in our Windows CI, by using a WebAssembly version of yoga-layout, as opposed to the asm.js version used by yoga-layout-prebuilt. This version of yoga-layout hasn't been published to npm yet, so I've vendored it in for now. We bundle this vendored version and redistribute it in our npm distribution. Memory profiling suggests Jest was allocating lots of memory to store this asm.js script.

As a side effect, this PR also fixes Linking failure in asm.js: Unexpected stdlib member warnings we've been seeing in CI tests.

I've tested wrangler dev (the only command that uses Ink and therefore yoga-layout afaik) and it seems to be working fine, including the hotkeys.

Associated docs issues/PR:

N/A

Author has included the following, where applicable:

  • Tests (attempt to fix existing tests)
  • Changeset (no user facing changes)

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@changeset-bot
Copy link

changeset-bot bot commented Jan 3, 2023

⚠️ No Changeset found

Latest commit: 255fac9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #2489 (255fac9) into main (8986ccb) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2489      +/-   ##
==========================================
+ Coverage   72.13%   72.17%   +0.03%     
==========================================
  Files         156      156              
  Lines        9720     9720              
  Branches     2546     2546              
==========================================
+ Hits         7012     7015       +3     
+ Misses       2708     2705       -3     
Impacted Files Coverage Δ
packages/wrangler/src/git-client.ts 81.25% <0.00%> (+4.16%) ⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts 100.00% <0.00%> (+5.26%) ⬆️

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/3837290055/npm-package-wrangler-2489

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2489/npm-package-wrangler-2489

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3837290055/npm-package-wrangler-2489 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3837290055/npm-package-cloudflare-pages-shared-2489

Fixes `Linking failure in asm.js: Unexpected stdlib member` warnings
@mrbbot mrbbot force-pushed the bcoll/yoga-layout-wasm branch from 7ae672d to 255fac9 Compare January 4, 2023 10:44
@mrbbot mrbbot marked this pull request as ready for review January 4, 2023 10:56
@mrbbot mrbbot requested a review from a team as a code owner January 4, 2023 10:56
@mrbbot mrbbot changed the title DO NOT MERGE: Update prebuilt yoga-layout from https://github.com/facebook/yoga Update prebuilt yoga-layout from https://github.com/facebook/yoga Jan 4, 2023
@penalosa
Copy link
Contributor

penalosa commented Jan 4, 2023

Of note—we still use Ink in a bunch of places (until #2283 is merged, which should hopefully be soon once I can fix the tests).

Why isn't this published to npm yet? I feel pretty hesitant about using a beta version of such a complex dependency. What's the timeline on stabilisation? Is there a tracking issue somewhere for Yoga?

@mrbbot
Copy link
Contributor Author

mrbbot commented Jan 4, 2023

@penalosa ah, didn't realise #2283 wasn't merged yet. FWIW, I did also try wrangler init and that worked too.

The important change in Yoga was only merged about a week ago: facebook/yoga#1177. The repository has seen much more activity recently (https://github.com/facebook/yoga/commits/main), but I'm not sure when this will be published to npm/stabilised. 😕

Whilst this dependency is complex, it is very well contained with a clear/concise API for interacting with it. Given this new version passes all Yoga's tests, and given we're having trouble landing new changes because of how frequently Windows CI fails, it might make sense to merge this now. Definitely see your point though...

Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

Things seem to be working, are we needing to wait on the Ink Removal PR?

Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

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

If this could wait until the Ink removal PR goes in (to limit the surface area), that'd be great. I have general concerns around the lack of maintenance Yoga appears to be getting, but if this works lets merge it (and hopefully 🤞 Yoga can be removed as a dependency soon)

@mrbbot mrbbot merged commit 415392a into main Jan 5, 2023
@mrbbot mrbbot deleted the bcoll/yoga-layout-wasm branch January 5, 2023 16:01
mrbbot added a commit that referenced this pull request Jan 5, 2023
#2489 broke the `Release` CI with an error that `patch-package`
couldn't be found when running `changeset-version.js`.

It looks like the `npm install` script in `changeset-version.js` is
only installing production dependencies. Moving `patch-package` to
`dependencies` leads to another error that `ink` cannot be found to
apply the patch to.

However, the only reason we run `npm install` here is to update
`package-lock.json`, so we don't need to be running `postinstall`
scripts. Therefore, this PR adds the `--package-lock-only` flag.
See https://docs.npmjs.com/cli/v9/commands/npm-install.
mrbbot added a commit that referenced this pull request Jan 6, 2023
…#2506)

#2489 broke the `Release` CI with an error that `patch-package`
couldn't be found when running `changeset-version.js`.

It looks like the `npm install` script in `changeset-version.js` is
only installing production dependencies. Moving `patch-package` to
`dependencies` leads to another error that `ink` cannot be found to
apply the patch to.

However, the only reason we run `npm install` here is to update
`package-lock.json`, so we don't need to be running `postinstall`
scripts. Therefore, this PR adds the `--package-lock-only` flag.
See https://docs.npmjs.com/cli/v9/commands/npm-install.
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.

4 participants