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

install using "optionalDependencies" #1621

Merged
merged 1 commit into from
Sep 22, 2021
Merged

install using "optionalDependencies" #1621

merged 1 commit into from
Sep 22, 2021

Conversation

evanw
Copy link
Owner

@evanw evanw commented Sep 20, 2021

This PR changes esbuild's installation strategy in an attempt to improve compatibility with edge cases such as custom registries, custom proxies, offline installations, read-only file systems, or when post-install scripts are disabled.

The old installation strategy manually downloaded the correct binary executable in a post-install script. The binary executable is hosted in a separate platform-specific npm package such as esbuild-darwin-64. The install script first attempted to download the package via the npm command in case npm had custom network settings configured. If that didn't work, the install script attempted to download the package from https://registry.npmjs.org/ before giving up. This was problematic for many reasons including:

  • Not all of npm's settings can be forwarded due to npm bugs such as [BUG] Config environment variable forwarding breaks with false config values npm/cli#2284, and npm has said these bugs will never be fixed.
  • Some people have configured their network environments such that downloading from https://registry.npmjs.org/ will hang instead of either succeeding or failing.
  • The installed package was broken if you used npm --ignore-scripts because then the post-install script wasn't run. Some people enable this option so that malicious packages must be run first before being able to do malicious stuff.

The new installation strategy automatically downloads the correct binary executable using npm's optionalDependencies feature to depend on all esbuild packages for all platforms but only have the one for the current platform be installed. This is a built-in part of the package manager so my assumption is that it should work correctly in all of these edge cases that currently don't work. And if there's an issue with this, then the problem is with the package manager instead of with esbuild so this should hopefully reduce the maintenance burden on esbuild itself. Changing to this installation strategy has these drawbacks:

  • Old versions of certain package managers (specifically npm and yarn) print lots of useless log messages during the installation, at least one for each platform other than the current one. These messages are harmless and can be ignored. However, they are annoying. There is nothing I can do about this. If you have this problem, one solution is to upgrade your package manager to a newer version.

  • Installation will be significantly slower in old versions of npm, old versions of pnpm, and all versions of yarn. These package managers download all packages for all platforms even though they aren't needed and actually cannot be used. This problem has been fixed in npm and pnpm and the problem has been communicated to yarn: [Bug?]: Yarn downloads irrelevant optional dependencies (e.g. for a different OS) yarnpkg/berry#3317. If you have this problem, one solution is to use a newer version of npm or pnpm as your package manager.

  • This installation strategy does not work if you use npm --no-optional since then the package with the binary executable is not installed. If you have this problem, the solution is to not pass the --no-optional flag when installing packages.

  • There is still a small post-install script but it's now optional in that the esbuild package should still function correctly if post-install scripts are disabled (such as with npm --ignore-scripts). This post-install script optimizes the installed package by replacing the esbuild JavaScript command shim with the actual binary executable at install time. This avoids the overhead of launching another node process when using the esbuild command. So keep in mind that installing with --ignore-scripts will result in a slower esbuild command.

Despite the drawbacks of the new installation strategy, I believe this change is overall a good thing to move forward with. It should fix edge case scenarios where installing esbuild currently doesn't work at all, and this only comes at the expense of the install script working in a less-optimal way (but still working) if you are using an old version of npm. So I'm going to switch installation strategies and see how it goes.

The platform-specific binary executables are still hosted on npm in the same way, so anyone who wrote code that downloads builds from npm using the instructions here should not have to change their code: https://esbuild.github.io/getting-started/#download-a-build. However, note that these platform-specific packages no longer specify the bin field in package.json so the esbuild command will no longer be automatically put on your path. The bin field had to be removed because of a collision with the bin field of the esbuild package (now that the esbuild package depends on all of these platform-specific packages as optional dependencies).

Related issues:

Fixes #286
Fixes #291
Fixes #319
Fixes #347
Fixes #369
Fixes #547
Fixes #565
Fixes #789
Fixes #921
Fixes #1193
Fixes #1270
Fixes #1382
Fixes #1422
Fixes #1450
Fixes #1485
Fixes #1546
Fixes #1547
Fixes #1574
Fixes #1609

@lukeed
Copy link
Contributor

lukeed commented Sep 21, 2021

Is there a way to test this? Are you looking for people to test this in their toolchains?

@evanw
Copy link
Owner Author

evanw commented Sep 21, 2021

Testing this out would be much appreciated. I have published a test package called esbuild-experimental-optdeps@0.12.10 which should be an accurate representation of how the esbuild package will behave after this PR lands. I have tested this with npm, pnpm, and yarn and everything seems to work.

However, it would be good to know if this works with unusual configurations such as custom registries, HTTP proxies, offline installations, etc. Right now I'm assuming it'll work since optionalDependencies is a long-standing existing feature of the npm ecosystem, but there could always be package manager bugs around this feature. The tracking issue for this PR is #789 so you can read that thread for more details and discussion about this approach.

I'm assuming that asking people to test this out prior to it landing isn't sufficient to flush out any issues with the approach since I've already asked before and very few people have actually tried it out. So as long as there aren't any blocker problems, I'm planning on merging this as a breaking change release to get wider feedback. But testing this out would still be very much appreciated as it's obviously better to uncover blocker issues before it lands.

@lukeed
Copy link
Contributor

lukeed commented Sep 22, 2021

Worked for me! I'm running pnpm@5.11.1 and it worked flawlessly in a regular repo and a monorepo environment (with multiple packages depending on esbuild-experimental-optdeps@0.12.10 directly).

My concern was that it was going to an experience similar to @swc/core installation, which downloads all binaries and then only keeps the matching package, but that wasn't the case 🎉

@evanw evanw merged commit 9e569c4 into master Sep 22, 2021
@evanw evanw deleted the issues/789 branch September 22, 2021 15:21
@hardfist
Copy link
Contributor

Worked for me! I'm running pnpm@5.11.1 and it worked flawlessly in a regular repo and a monorepo environment (with multiple packages depending on esbuild-experimental-optdeps@0.12.10 directly).

My concern was that it was going to an experience similar to @swc/core installation, which downloads all binaries and then only keeps the matching package, but that wasn't the case 🎉

pnpm has already fixes the bug since latest v6 verison and downloads only partial binaries of @swc/core

@lukeed
Copy link
Contributor

lukeed commented Sep 22, 2021

@hardfist right, but I wanted to see if the non-patched version treated this PR the same way

@danvim
Copy link

danvim commented Sep 27, 2021

Can confirm this (0.13.2) is working for me behind proxy. Coming from #319 🥳

I no longer need to prime the installation process by pre-downloading the latest version without the --prefer-offline option if I had previously installed esbuild at an older version.

@silverwind
Copy link

Can also confirm this as working in cases where it previously failed with a network timeout trying to contact the registry directly. 🎉

eduardoboucas pushed a commit to netlify/esbuild that referenced this pull request Dec 3, 2021
* fix evanw#1327: improve lowered template literals

* fix(linker): order of css imported from js (evanw#1342)

* release notes for evanw#1342

* update compat-table

* fix for "export default class" transform (evanw#1346)

* publish 0.12.6 to npm

* add support for es5-style identifiers (evanw#1349)

* runtime: remove "__platform" flag

* runtime: remove "__profiler" flag

* runtime: check "for-of" not "=>" for es6 support

* fix evanw#1349: quote modern unicode object properties

* fix evanw#1355: ignore tsconfig.json in node_modules

* fix evanw#1357: "--metafile" with "--watch"

* fix(linker): add missing esm flag (evanw#1338)

* Allow OnResolve plugins to mark modules as side effect free (evanw#1313)

* publish 0.12.7 to npm

* fix evanw#1358: remove warning about source map comment

* publish 0.12.8 to npm

* fix evanw#1361: allow "this" with "--define"

* fix evanw#1372: css minification bug with !important

* publish 0.12.9 to npm

* avoid checking "browser" for other platforms

* add an "es2021" target

* Avoid exporting a pointer to a loop variable in linker (evanw#1389)

The Bazel nogo (Go lint config) errored when I tried to compile esbuild:

    compilepkg: nogo: errors found by nogo during build-time code analysis:
    external/com_github_evanw_esbuild/internal/bundler/linker.go:3309:27:
     exporting a pointer for the loop variable stmt (export_loop_ref)

The simplified code nogo complains about is:

    for _, stmt := range partStmts {
      stmt.Data = &js_ast.SImport{
        StarNameLoc: &stmt.Loc,
      }
    }

The problem is `&stmt.Loc` points to the mutated loop variable `stmt`.  After
the loop iteration ends, all stored pointers will point to the last value of
`partStmts[-1].Loc`.

An alternative solution is to shadow `stmt` at the beginning of the loop, but
this felt cleaner:

    stmt := stmt

The lint rule is defined by https://github.com/kyoh86/exportloopref.

* feat: mangle Infinity (evanw#1385)

* add support for shorten transform/translate3d (evanw#1390)

* css: implement minification for all matrix forms

* fix evanw#1397: support "s" in css attribute selectors

* publish 0.12.10 to npm

* fix evanw#1399: avoid "os.MkdirAll" to fix WebAssembly

* fix evanw#1396: improve invalid loader error message

* improve sync performance of js api by ~20x (evanw#1000)

* fix windows issues

* publish 0.12.11 to npm

* move unique key prefix from compile to scan phase

* add "C" to unique keys for chunks

* fix evanw#1044: correct relative paths for file loader

* fix a windows path issue

* publish 0.12.12 to npm

* Fix using JS synchronous API from from non-main threads (evanw#1411)

* publish 0.12.13 to npm

* keep wasm tests self-contained

* factor out some code related to "outfile"

* pull out relative-to-outbase code

* fix evanw#1404: "file" loader always copies to "outdir"

* publish 0.12.14 to npm

* fix evanw#1421: bug with css color lowering and "var()"

* avoid "var()" issues with other css minifications

* publish 0.12.15 to npm

* update the compat table

* allow out-of-range tagged template unicode escapes

* fix evanw#1426: remove warning about bad CSS "@" rules

* fix evanw#1470: allow "ES2021" in "tsconfig.json"

* fix evanw#1462: avoid worker_threads in node <v12.17.0

* fix evanw#1466: paths with "node:" prefix are external

* Consider `\` and `/` to be the same in file paths (evanw#1472)

* publish 0.12.16 to npm

* fix evanw#1455: bundler hoisting bug with var+for loops

* fix evanw#1418: private fields and logical assignment

* Abort esbuild if stdin is closed when serving (evanw#1449)

* release notes for evanw#1449

* fix evanw#1424: always generate private method names

* publish 0.12.17 to npm

* fix evanw#1483: UTF-8 and utf-8 are the same @charset

* improve error about missing sub-condition (evanw#1484)

* refactor(deno): use denoflate instead of compress (evanw#1482)

deno.land/x/denoflate is about 10% smaller, and a lot more polished and
up to date than deno.land/x/compress.

* fix evanw#1493: nullish coalescing assignment edge case

* fix evanw#1489: do not warn about "es3" in node_modules

* fix evanw#1497: "this" before "super()" when minifying

* avoid shadowing "expr" in "lowerClass"

* fix evanw#1498: variable shadowing broke class lowering

* fix: CSS import relative paths (evanw#1494)

* add release notes for evanw#1494

* publish 0.12.18 to npm

* move source map code to source map module

* css: add location info to rules

* css: printer returns result object

* move span object to logger

* css: add support for source maps

* add extension to source map tests

* add a basic css source map test

* fix evanw#519: release notes for css source maps

* fix evanw#1507: wrong ts class field side effect order

* publish 0.12.19 to npm

* avoid printing "</style" in CSS code (evanw#1509)

* attempt to fix flaky test

* update browser compat data

* fix evanw#1512: asi issue with "." and type parameters

* fix evanw#1509: make `</script` escape case-insensitive

* publish 0.12.20 to npm

* update to go version 1.17.0

* fix evanw#995: windows arm64 support

* run go format from go 1.17.0

* css: terminate source map comment before "*/"

* add windows 64-bit arm build to installer (evanw#995)

* publish 0.12.21 to npm

* fix evanw#1536: http range requests now use less memory

* fix evanw#1538: minify bug for "var()" and "box-shadow"

* publish 0.12.22 to npm

* fix evanw#1553: rest bindings in TypeScript arrow types

* fix evanw#1545: "watch" is not allowed with "buildSync"

* fix evanw#1552: keep names + minify + nested functions

* forbid "watch" w/ "buildSync" w/o "worker_threads"

* publish 0.12.23 to npm

* fix direct "eval" variable renaming edge case

* publish 0.12.24 to npm

* fix evanw#1560: bug with "!" after "new" in TypeScript

* capture and report parser panics

* fix parser panic due to "#a in #b in c"

* class static blocks are a parse error

* illumos 64-bit support (evanw#1562)

* release notes for evanw#1562

* publish 0.12.25 to npm

* feat: Optimizing the __require function (evanw#1580)

* release notes for evanw#1579

* move "NO_COLOR" handling into the logger itself

* add an "analyze metafile" api

* add import paths to analysis

* add a "verbose" flag to analysis

* fix evanw#1568: release notes for "--analyze"

* upgrade "golang.org/x/sys" (evanw#1572)

* publish 0.12.26 to npm

* fix evanw#1594: update manual compat table overrides

* replace math.MaxInt usage (evanw#1585)

This constant is only available in go >= 1.17, so I've inlined its value
so dependents don't have to upgrade their go version.

reference implementation: https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/math/const.go;l=38

* fix evanw#1589: server "stop()" waits for active builds

* use "math.MaxUint32" not "math.MaxInt"

* update go 1.17.0 => go 1.17.1

* publish 0.12.27 to npm

* fix evanw#1599: U+30FB and U+FF65 in ES5 vs. ES6+

* fix evanw#1600: "++" and "--" on class private fields

* publish 0.12.28 to npm

* fix evanw#1614: proxy from "__require" to "require"

* fix evanw#1623: ignore class fields marked "abstract"

* "typeof identifier" has no side effects

* fix "__require" to have no side effects

* fix mangle syntax edge case with "==" and "!="

* fix missing return in "IsNumericValue"

* add "--analyze" to cli help text

* publish 0.12.29 to npm

* no side effects for "typeof x != undefined && x"

* separate "ignore annotations" from "tree shaking" (evanw#1625)

* install using "optionalDependencies" (evanw#1621)

* release notes

* publish 0.13.0 to npm

* fix release gh action to ignore nested headers

* fix the "esbuild" package in yarn 2+

* yarn pnp compat: copy binary into the current pkg

* publish 0.13.1 to npm

* fix evanw#1628: "export {}" with "--tree-shaking=true"

* fix cache condition in iswin_wasm (evanw#1630)

* publish 0.13.2 to npm

* add "preferUnplugged: false" to binary packages

This is a yarn-specific "package.json" flag and is being added at the recommendation of the Yarn team. Even though esbuild's binary packages are listed as optional dependencies of the main package, Yarn still installs all of them (even though only one applies to the current platform). And unlike npm, which always installs a given package into a directory on the file system, Yarn can represent a given package either as a zip file or as a directory of files. So ideally as many packages as possible are represented as zip files to minimize wasted space on the file system (since zip files are compressed). One of the heuristics that Yarn uses is to represent a package as a directory if it contains a file ending in ".exe" so unfortunately esbuild's three Windows packages are always stored as directories instead of as zip files, which means they are uncompressed and are larger than necessary. Specifying "preferUnplugged: false" should avoid this. Hopefully someday Yarn won't even install these packages on the file system in the first place to eliminate the wasted space completely.

See also:

* https://yarnpkg.com/configuration/manifest/#preferUnplugged
* yarnpkg/berry#3317 (comment)

* support type-only import/export specifiers (evanw#1637)

* publish 0.13.3 to npm

* fix evanw#1642: permission issues with install script

* basic support for ".mts" and ".cts" from TS 4.5

* fix evanw#1647: add a fallback for "npm --no-optional"

* make pnpapi workaround platform-specific (evanw#1656)

I'm not sure if this will fix anything, but it probably couldn't hurt.

* no optimizations with yarn 1 just in case (evanw#1656)

* fix evanw#1657: invalid css transform of margin/padding

* remove ".mts" and ".cts" from resolve extensions

* publish 0.13.4 to npm

* fix evanw#1113: improve watch mode accuracy (evanw#1676)

* disallow certain "<" in ".mts/.cts" files

* fix evanw#1665: don’t remove empty @Keyframes (evanw#1669)

* release notes for evanw#1665

* Don't emit "duplicate label" error across function scopes. (evanw#1671)

* release notes for evanw#1671

* publish 0.13.5 to npm

* Add NetBSD amd64 binary (evanw#1624)

* https in changelog, rebalance makefile

* Allow bundled esbuild with ESBUILD_BINARY_PATH (evanw#1678)

* feat: drop catch binding when optional catch binding is supported (evanw#1660)

* fix subtle minify issues with eval

* ts: forbid "declare" fields from being initialized

* ts: forbid "declare" on non-field class properties

* fix evanw#1675: run decorators for "declare" fields

* avoid direct eval retaining unused imports in ts

* publish 0.13.6 to npm

* update parcel 2 version in benchmark

* remove now-unnecessary "@parcel/transformer-typescript-tsc"

* remove old bundler versions

* update rollup and webpack too

* update benchmark image

* fix evanw#1682: always use the shortest css alpha value

* fix evanw#1680: match node's core module behavior

* update go 1.17.1 => 1.17.2

* fix wasm on go 1.17.2 (evanw#1684)

* update rollup tests so they work on node v16.11.1

* publish 0.13.7 to npm

* fix evanw#1425: super inside arrow inside lowered async

* add "and CSS" to package description

* fix evanw#1661: remove implicit trailing "/" in "[dir]"

* add a test for evanw#1362

* publish 0.13.8 to npm

* fix evanw#1702: invalid css transform of border-radius

* make yaml formatting consistent

* add simple end-to-end tests

* fix evanw#1703: handle silent "rename" syscall failure

* add pnpm end-to-end tests

* check end-to-end test output

* resolver: rename "pe" => "pj"

* remove unused range

* fix evanw#1691: support "imports" in "package.json"

* publish 0.13.9 to npm

* yarn berry end-to-end test

* try running end-to-end tests on github

* check that esbuild builds on go 1.13

* Use `io.SeekStart` instead of deprecated `os.SEEK_SET` (evanw#1701)

`os.SEEK_SET` has been deprecated since Go 1.7.
Ref: https://pkg.go.dev/os#pkg-constants

* add "check out code" to old go version ci

* update @next targets for npm and yarn

* link from code to docs for vs code autocomplete

* remove invalid "es7" option in tsconfig parser

* update the compat table

* Allow target for ES-Version to be uppercase (evanw#1718)

* fix evanw#1539: implement legal comments for css

* update to unicode 14

* add ".mts" and ".cts" to exports kind checking

* publish 0.13.10 to npm

* reorder some functions

* get tests working on node 17+

* also run async transform tests un-transformed

* run tests w/ node 16 not 14 to avoid hard-crash

Node 14 has some bug that results in an "unreachable code" panic. For the record, the traceback is as follows:

     1: node::NodePlatform::GetStackTracePrinter()::$_3::__invoke()
     2: V8_Fatal(char const*, ...)
     3: v8::internal::interpreter::BytecodeGenerator::VisitCompoundAssignment(v8::internal::CompoundAssignment*)
     4: v8::internal::interpreter::BytecodeGenerator::VisitNoStackOverflowCheck(v8::internal::AstNode*)
     5: v8::internal::interpreter::BytecodeGenerator::GenerateBytecodeBody()
     6: v8::internal::interpreter::BytecodeGenerator::GenerateBytecode(unsigned long)
     7: v8::internal::interpreter::InterpreterCompilationJob::ExecuteJobImpl()
     8: v8::internal::(anonymous namespace)::ExecuteSingleUnoptimizedCompilationJob(v8::internal::ParseInfo*, v8::internal::FunctionLiteral*, v8::internal::AccountingAllocator*, std::__1::vector<v8::internal::FunctionLiteral*, std::__1::allocator<v8::internal::FunctionLiteral*> >*)
     9: v8::internal::(anonymous namespace)::IterativelyExecuteAndFinalizeUnoptimizedCompilationJobs(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SharedFunctionInfo>, v8::internal::Handle<v8::internal::Script>, v8::internal::ParseInfo*, v8::internal::AccountingAllocator*, v8::internal::IsCompiledScope*, std::__1::vector<v8::internal::FinalizeUnoptimizedCompilationData, std::__1::allocator<v8::internal::FinalizeUnoptimizedCompilationData> >*)
    10: v8::internal::Compiler::Compile(v8::internal::Handle<v8::internal::SharedFunctionInfo>, v8::internal::Compiler::ClearExceptionFlag, v8::internal::IsCompiledScope*)
    11: v8::internal::Compiler::Compile(v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Compiler::ClearExceptionFlag, v8::internal::IsCompiledScope*)
    12: v8::internal::Runtime_CompileLazy(int, unsigned long*, v8::internal::Isolate*)
    13: Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit
    14: Builtins_CompileLazy
    15: Builtins_InterpreterEntryTrampoline

* also run class lowering tests untransformed

* test coverage for super and object methods

* fix writing to a "super" property

* also handle "super" inside static class fields

* implement class static blocks (evanw#1729)

* publish 0.13.11 to npm

* enable tree shaking for empty "static {}" blocks

* fix evanw#1730: crash with legal comment and @import

* enable tree shaking of "Reflect" static methods

* implement "calc()" reduction for css (evanw#1731)

* publish 0.13.12 to npm

* fix evanw#1739: tree shaking bug with "var exports"

* border radius tests: use length instead of number

* Add css to help text for --loader (evanw#1744)

* allow empty string for CLI string arrays

* move "main fields" logic to a separate function

* make debug meta available to the entire resolver

* say if "main" is missing from main fields (evanw#1754)

* fix evanw#1755: merge adjacent selectors with same body

* add spack to benchmarks (not ready due to bugs)

* Shorten "top", "right" properties into "inset" property (evanw#1758)

* add credit to changelog

* publish 0.13.13 to npm

* chore: make build pass

* chore: update publishing scripts

Co-authored-by: Evan Wallace <evan.exe@gmail.com>
Co-authored-by: dmitrage <dmitrage@gmail.com>
Co-authored-by: Liu Bowen <Mr_lbw@outlook.com>
Co-authored-by: Chris Casola <chriscasola@gmail.com>
Co-authored-by: Joe Schafer <joe@jschaf.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: Weilin Shi <934587911@qq.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: Luca Casonato <hello@lcas.dev>
Co-authored-by: Rongjian Zhang <pd4d10@gmail.com>
Co-authored-by: Dominik Hassler <hadfl@omnios.org>
Co-authored-by: FM <15306225869@163.com>
Co-authored-by: David Zukowski <david@zuko.me>
Co-authored-by: John Doe <you@example.com>
Co-authored-by: Georges Varouchas <georges.varouchas@gmail.com>
Co-authored-by: Pig Fang <g-plane@hotmail.com>
Co-authored-by: Eelco Lempsink <eelco@framer.com>
Co-authored-by: Nevkontakte <nevkontakte@users.noreply.github.com>
Co-authored-by: Greg Troxel <gdt@lexort.com>
Co-authored-by: Piotr Krawiec <piotr@krawiec.me>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: y-yagi <yuuji.yaginuma@gmail.com>
Co-authored-by: timse <tim.sebastian@gmail.com>
Co-authored-by: Dan Rosén <danr42@gmail.com>
Co-authored-by: Netlify Team Account 1 <netlify-team-account-1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment