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

refactor: update esbuild and remove esbuild-plugin-pnp #4732

Merged
merged 9 commits into from Sep 2, 2022

Conversation

RDIL
Copy link
Member

@RDIL RDIL commented Aug 10, 2022

What's the problem this PR addresses?
esbuild v0.15.0 brings native support for pnp, so this PR deprecates the plugin and updates dependencies within the monorepo.

See also: evanw/esbuild#2451
Closes #4787

How did you fix it?
Updated versions, removed the plugin.

Checklist

  • I have set the packages that need to be released for my changes to be effective.

Note: I wasn't entirely sure what should be released for this, feel free to give any suggestions as to what should be major/minor/patch/declined.

  • I will check that all automated PR checks pass before the PR gets reviewed.

@RDIL RDIL requested a review from arcanis as a code owner August 10, 2022 16:36
@RDIL RDIL force-pushed the esbuild branch 2 times, most recently from 42e2ce0 to c459fb9 Compare August 10, 2022 16:44
@merceyz
Copy link
Member

merceyz commented Aug 10, 2022

Blocked until evanw/esbuild#2453 is released.

@RDIL
Copy link
Member Author

RDIL commented Aug 10, 2022

@merceyz is that why the CLI build is failing?

fatal error: too many writes on closed pipe

goroutine 6 [running]:
runtime.throw({0x93bec, 0x1e})
        runtime/panic.go:1047 +0x3 fp=0x83dee0 sp=0x83deb8 pc=0x122d0003
os.sigpipe()
        runtime/os_js.go:143 +0x2 fp=0x83def8 sp=0x83dee0 pc=0x139d0002
os.epipecheck(...)
        os/file_unix.go:195
os.(*File).Write(0x80c020, {0x5fa5fc0, 0x21, 0x40})
        os/file.go:183 +0x27 fp=0x83df80 sp=0x83def8 pc=0x15e70027
main.runService.func1()
        github.com/evanw/esbuild/cmd/esbuild/service.go:114 +0xa fp=0x83dfe0 sp=0x83df80 pc=0x1e9e000a
runtime.goexit()
        runtime/asm_wasm.s:401 +0x1 fp=0x83dfe8 sp=0x83dfe0 pc=0x13ec0001
created by main.runService
        github.com/evanw/esbuild/cmd/esbuild/service.go:108 +0x13

goroutine 1 [chan receive]:
runtime.gopark(0xa9328, 0x975d98, 0xe, 0x17, 0x2)
        runtime/proc.go:363 +0x27 fp=0x861980 sp=0x861958 pc=0x12540027
runtime.chanrecv(0x975d40, 0x86fad0, 0x1)
        runtime/chan.go:583 +0x7c fp=0x861a08 sp=0x861980 pc=0x1065007c
runtime.chanrecv1(0x975d40, 0x86fad0)
        runtime/chan.go:442 +0x2 fp=0x861a30 sp=0x861a08 pc=0x10630002
syscall.fsCall({0x85fab, 0x4}, {0x86fba8, 0x5, 0x5})
        syscall/fs_js.go:520 +0x13 fp=0x861b00 sp=0x861a30 pc=0x15850013
syscall.Read(0x0, {0x8da000, 0x4000, 0x4000})
        syscall/fs_js.go:388 +0xb fp=0x861c00 sp=0x861b00 pc=0x1581000b
internal/poll.ignoringEINTRIO(...)
        internal/poll/fd_unix.go:794
internal/poll.(*FD).Read(0x832060, {0x8da000,

@paul-soporan
Copy link
Member

IMO we should first release a new version of @yarnpkg/esbuild-plugin-pnp that checks the version of ESBuild and:

@merceyz
Copy link
Member

merceyz commented Aug 11, 2022

is that why the CLI build is failing?

@RDIL I checked but no, seems to be something else, reported upstream in evanw/esbuild#2458

@merceyz
Copy link
Member

merceyz commented Aug 17, 2022

I can now successfully build and run Yarn on both Windows and Linux with esbuild-wasm@0.15.5 however the build is a lot slower, yarn build:cli on master takes 12s 111ms while in this PR it takes 26s 975ms.
@evanw in https://github.com/evanw/esbuild/blob/a6c42a19ff62a70d118e8e016a302fcac84ed95f/CHANGELOG.md#0150 you mention over a 10x speedup (3.4s to 0.24s), do you still see those improvements?


I'll convert this PR into a draft until it's in a mergeable state.

@merceyz merceyz marked this pull request as draft August 17, 2022 13:36
@evanw
Copy link

evanw commented Aug 17, 2022

@evanw in https://github.com/evanw/esbuild/blob/a6c42a19ff62a70d118e8e016a302fcac84ed95f/CHANGELOG.md#0150 you noticed over a 10x speedup (3.4s to 0.24s), do you still see those improvements?

Yes I do, but I suspect you are running esbuild in the slowest possible way. You are using the WebAssembly version instead of the native version, which is typically 10x slower, and then you are also running esbuild through Yarn, which adds an additional performance overhead.

To demonstrate the performance overhead of the different ways of calling esbuild, I'm building the Yarn CLI using the following command:

esbuild --bundle packages/yarnpkg-cli/sources/cli.ts --platform=node --outfile=out.js

Here's a table of the resulting times (each is the best of three runs, done in a Linux VM):

Approach Time Slowdown
esbuild (native) 0.587 seconds 1x
yarn esbuild (native) 7.104 seconds 12.1x slower
esbuild (wasm) 12.684 seconds 21.6x slower
yarn esbuild (wasm) 19.681 seconds 33.5x slower

@merceyz
Copy link
Member

merceyz commented Aug 17, 2022

Thanks for confirming, I tested again because the results I got were a bit odd, now it's consistently 5s 831ms vs 8s 154ms without changing anything so I'm assuming the previously slow runs were something else causing it.

@arcanis
Copy link
Member

arcanis commented Sep 2, 2022

It seems the failing tests are triggered by tau-prolog defining a global variable the old way, and Esbuild now adding use strict at the top of the generated bundle. I've set alwaysStrict: false which should revert to the old behaviour (no use strict), although it'd be better to get tau-prolog to fix it on their side.

@merceyz merceyz changed the title refactor: Deprecate esbuild-plugin-pnp refactor: update esbuild and remove esbuild-plugin-pnp Sep 2, 2022
@arcanis arcanis marked this pull request as ready for review September 2, 2022 15:30
@arcanis arcanis merged commit 4c8ab43 into yarnpkg:master Sep 2, 2022
@RDIL RDIL deleted the esbuild branch September 3, 2022 21:30
merceyz added a commit that referenced this pull request Sep 28, 2022
* refactor: Deprecate esbuild-plugin-pnp

* Update e2e workflow

* Update .yarn/versions/ded9c7c0.yml

Co-authored-by: Kristoffer K. <merceyz@users.noreply.github.com>

* deps: `esbuild-wasm@0.15.3`

* deps: `esbuild-wasm@0.15.5`

* chore: remove `baseUrl`

* Fixes use strict

* Restores the readme to explain the deprecation

Co-authored-by: Kristoffer K. <merceyz@users.noreply.github.com>
Co-authored-by: Maël Nison <nison.mael@gmail.com>
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.

[Bug?]: Update esbuild version in @yarnpkg/builder
5 participants