Resolver: recursive remaps (browser & tsconfig)

[AaronO]

Problem

esbuild (unlike webpack and tsc I believe) does not currently allow remaps (i.e: rewriting imports) for submodules that define their own remaps even if there are no conflicts/overlaps between remaps from parent and child modules.

This happens because esbuild resolves remaps (in a submodule) by using the remap context closest defined to the submodule.

Example problem

Remapping react-native to react-native-web in the entrypoint/app module and importing a submodule that uses react-native and defines its own (unrelated) tsconfig.compilerOptions.paths.

Similar issues happen for package.json's .browser.

Solution

Enable the resolver to recursively lookup remaps in parent contexts.

This changes the effective remap context (in pseudocode) from:

remap_ctx = child || parent || ... || nth_parent

to:

remap_ctx = merge(nth_parent, ... , parent, child)

Effectively a merge (or union) of contexts with child-keys (closest to the submodule) taking precedence.

I don't see any downside to "merging" contexts vs the current "fallback" approach, since the "merged context" is a superset of the current "fallback context" (do let me know if I'm missing something)

Fixes #238

Tests with & without fixes

I've added two unit-tests TestPackageJsonBrowserRecursive and TestTsConfigPathsRecursive that fail using resolver.go from master and pass using this branch's resolver.go

~/git/esbuild resolver/recursive-remaps
❯ make test-go
go test ./internal/...
ok  	github.com/evanw/esbuild/internal/ast	(cached)
ok  	github.com/evanw/esbuild/internal/bundler	(cached)
?   	github.com/evanw/esbuild/internal/compat	[no test files]
?   	github.com/evanw/esbuild/internal/config	[no test files]
ok  	github.com/evanw/esbuild/internal/fs	(cached)
ok  	github.com/evanw/esbuild/internal/lexer	(cached)
?   	github.com/evanw/esbuild/internal/logging	[no test files]
ok  	github.com/evanw/esbuild/internal/parser	(cached)
ok  	github.com/evanw/esbuild/internal/printer	(cached)
?   	github.com/evanw/esbuild/internal/resolver	[no test files]
?   	github.com/evanw/esbuild/internal/runtime	[no test files]
?   	github.com/evanw/esbuild/internal/sourcemap	[no test files]
?   	github.com/evanw/esbuild/internal/test	[no test files]

~/git/esbuild resolver/recursive-remaps
❯ git checkout master internal/resolver/resolver.go
Updated 1 path from 3882122

~/git/esbuild resolver/recursive-remaps*
❯ make test-go
go test ./internal/...
ok  	github.com/evanw/esbuild/internal/ast	(cached)
--- FAIL: TestPackageJsonBrowserRecursive (0.00s)
    --- FAIL: TestPackageJsonBrowserRecursive/#00 (0.00s)
        bundler_test.go:38: +/Users/user/project/node_modules/demo-pkg/index.js: error: Could not resolve "https"

--- FAIL: TestTsConfigPathsRecursive (0.00s)
    --- FAIL: TestTsConfigPathsRecursive/#00 (0.00s)
        bundler_test.go:38: +/Users/user/project/node_modules/worldview/index.tsx: error: Could not resolve "react-native"

FAIL
snapshots/snapshots_default.txt
    No test found for snapshot TestPackageJsonBrowserRecursive
snapshots/snapshots_tsconfig.txt
    No test found for snapshot TestTsConfigPathsRecursive
FAIL	github.com/evanw/esbuild/internal/bundler	0.147s
?   	github.com/evanw/esbuild/internal/compat	[no test files]
?   	github.com/evanw/esbuild/internal/config	[no test files]
ok  	github.com/evanw/esbuild/internal/fs	(cached)
ok  	github.com/evanw/esbuild/internal/lexer	(cached)
?   	github.com/evanw/esbuild/internal/logging	[no test files]
ok  	github.com/evanw/esbuild/internal/parser	(cached)
ok  	github.com/evanw/esbuild/internal/printer	(cached)
?   	github.com/evanw/esbuild/internal/resolver	[no test files]
?   	github.com/evanw/esbuild/internal/runtime	[no test files]
?   	github.com/evanw/esbuild/internal/sourcemap	[no test files]
?   	github.com/evanw/esbuild/internal/test	[no test files]
FAIL
make: *** [test-go] Error 1

[AaronO]

@evanw Have you had time to look at this ?

The tsconfig resolution issue is a regression preventing us from upgrading (0.5.25 => 0.6.20), the tsconfig resolution used to be recursive (.browser possibly never was).

Looking at the resolver code from v0.5.25: https://github.com/evanw/esbuild/blob/v0.5.25/internal/resolver/resolver.go#L891-L925

We can see that the entirety of resolver.loadNodeModules() (which does the tsconfig remapping) was recursive, but in 0.6.20 (https://github.com/evanw/esbuild/blob/v0.6.20/internal/resolver/resolver.go#L999-L1035) the tsconfig part is out of the recursive loop.

[evanw]

It looks like this PR is actually for two separate features:

1. Nested browser override maps

   I just finished investigating this feature. In my testing it appears that at least Parcel also does this, so this seems like a good feature to include in esbuild for compatibility.

2. Nested tsconfig.json override paths

   It's not immediately obvious to me that this change is something that should be included in esbuild. What other tools behave like this? Is there an existing precedent for this? I'm asking because I'm trying to have esbuild follow existing tooling conventions without inventing new ones to maximize compatibility. One issue I came across while searching for existing implementations was this one in Webpack's ts-loader plugin, but it was closed without resolution suggesting to me that this isn't supported.

   I think the way people normally use multiple tsconfig.json is to have the ones in nested folders extend the one in the root folder using the extends field. This should already work with esbuild since esbuild supports the extends field. Is there a reason this doesn't work for your use case?

   I noticed your test case is testing packages in node_modules with tsconfig.json files, but both the react-native-web and worldview packages don't actually publish .tsx files to npm. So I'm confused about what the intended behavior is. AFAIK it's very unusual to publish TypeScript files to npm because there isn't an expectation that consumers of your package will be transpiling TypeScript files.

[AaronO]

It looks like this PR is actually for two separate features:

They are indeed different mechanisms, but I grouped them together since they both concern import rewrites/remaps and the issue is not that they don't work, but that both do not work recursively (so the PR was broadly about making all rewrite/remap logic work recursively).

Happy to split them out, especially if there is more debate around the tsconfig part.

2. Nested tsconfig.json override paths

To give some context, prior to esbuild we were using webpack and .resolve.alias to rewrite/replace imports of "unaware" submodules (react-native => react-native-web, to allow using modules built on that shared API-surface but weren't necessarily designed with react-native-web in mind).

The team member who originally replaced webpack with esbuild(@0.5.25 at the time), switched those webpack.resolve.alias to tsconfig.compilerOptions.paths and it worked (because as of @0.5.25 those tsconfig remaps in esbuild were resolved recursively), but is now "broken" since 0.6.6 (#278, 3e027f6)

Ultimately we need(ed) a mechanism for "deep" rewrites of imports used by modules we do not control (e.g: deps of deps installed from NPM), similar to what webpack's .resolve.alias enabled (not limited to browser bundles like .browser)

I noticed your test case is testing packages in node_modules with tsconfig.json files, but both the react-native-web and worldview packages don't actually publish .tsx files to npm. So I'm confused about what the intended behavior is. AFAIK it's very unusual to publish TypeScript files to npm because there isn't an expectation that consumers of your package will be transpiling TypeScript files.

Sorry if that was confusing, for the purpose of this issue it doesn't matter wether those files are .js or .tsx? since esbuild will parse both and apply tsconfig.compilerOptions.paths in both cases.

This comment is a bit of a brain dump (hopefully it provides context and clears up a few things), I'll follow-up with a cleaner comment on the core issue on how it could be fixed.

[evanw]

Ultimately we need(ed) a mechanism for "deep" rewrites of imports used by modules we do not control (e.g: deps of deps installed from NPM), similar to what webpack's .resolve.alias enabled (not limited to browser bundles like .browser)

FWIW I'm planning for resolver plugins to address more advanced path alias use cases. In addition to supporting this use case, they should also be able to support even more advanced use cases that require actually executing JavaScript code to do path resolution that can't be represented as a map (e.g. Yarn PnP). This approach should be fully general-purpose.

[AaronO]

Recap of the previous comment: we needed a mechanism for deep remaps with esbuild (similar to webpack's .resolve.alias or rollup's alias plugin). esbuild's application of tsconfig.compilerOptions.paths used to provide that and still does (on master / 0.6.20) as long as the sub-module has no tsconfig.json of its own.

I've put together a small repo to reproduce this stuff: https://github.com/AaronO/esbuild-issue-tsconfig

The conclusion there is that esbuild behaves inconsistently:

• It builds successfully (by applying the tsconfig remaps) in the absence of a tsconfig.json in the sub-module
• It fails to build when the submodule has a tsconfig.json

Unfortunately it's common for published NPM modules to ship their tsconfig.json (run find node_modules -type f -name tsconfig.json on a largish JS/TS project) so the current behaviour is unreliable.

Root problems

1. esbuild needs a means for users to express "deep" remaps, similar to webpack's .resolve.alias
2. The current implementation (and mine) doesn't have the same behavior as webpack+ts-loader

If we define common categories of remap contexts as following:

• direct: remap_ctx = child || nil
• fallback: remap_ctx = child || parent || ... || nth_parent
• merged: remap_ctx = merge(nth_parent, ... , parent, child)

esbuild's implementation is effectively fallback whereas ts-loader is direct, .browser is merged.

fallback has the downside of sometimes behaving (for the end-user) as merged or direct, which can be perceived as annoyingly inconsistent.

If we want to follow existing conventions we should correct that (turn it into direct like ts-loader)

I was pushing to make it behave like merged as that would give us a means to express deep remaps without adding a new concept/config, whilst being more consistent than the current behaviour and a superset of the established behaviour.

But your comment on custom esbuild loaders makes sense, is probably much cleaner and avoids headaches that could come from diverging from established behavior of .paths.

[AaronO]

Sorry for all that noisy context, I'll split out the browser part into a different PR.

FWIW I'm planning for resolver plugins to address more advanced path alias use cases. In addition to supporting this use case, they should also be able to support even more advanced use cases that require actually executing JavaScript code to do path resolution that can't be represented as a map (e.g. Yarn PnP). This approach should be fully general-purpose.

That sounds great, do you have an ETA in mind ? Anything we could help with ?

BTW, I was also using these "deep remaps" to workaround esbuild erroring on submodule sourcemaps (e.g: es6-promise) with negative column-deltas. But I think that could be solved by implementing "relaxed" binary-search and allowing a certain tolerance of negative column-deltas.