Build: fully resolve import paths in transpiled files

[jsnajdr]

This PR implements the same thing as #73422, but as part of esbuild onResolve callback, without a separate post-processing step with Babel.

Local imports like import from './proxy' are resolved either to './proxy.js' or './proxy/index.js', depending on which file/directory exists. That way the imports become compatible with Node.js ESM import rules, and also with browsers' native ESM import rules. They both require full file paths, without any shortcuts.

I originally wanted to eliminate local imports completely in #73516, but that proved too ambitious and risky.

This esbuild approach is very simple and flexible. If we want to, we can easily modify it to make all imports use the .mjs or .cjs extension. That will be useful for creating fully compliant dual CJS/ESM packages.

Before we merge this, I'd like to fine-tune the algorithm a bit. Make sure that special paths like '.' and '..' are handled correctly, that imports with an existing non-JS extension (like block.json) are not handled with this code, etc. But I didn't want to delay the PR further. Let's test and review.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>
Co-authored-by: dmsnell <dmsnell@git.wordpress.org>
Co-authored-by: anomiex <bjorsch@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[youknowriad]

I'm actually a little bit surprised that we have to write it manually ourselves, I've kind expected esbuild (or some plugin) to do it for us already as it feels like a very common thing for folks building packages these days.

[jsnajdr]

You are not alone who is surprised how tough it is to build fully compliant dual CJS/ESM TypeScript packages and how unprepared the common tools are. There are specialized tools like duel or tsup which are rather complex and whose inner workings are non-trivial to reproduce in a build script like ours.

Earlier this year I worked on a dual package in the Jetpack repo. Initially I tried to do my own build script, but eventually gave up and just used duel.

[aduth]

I think ESBuild considers relations between files primarily around bundling, which isn't how we're using it. Although to a some extent, JavaScript / Node.js is shifting toward import paths including file extensions, where this behavior of automatic extension resolution is completely unsupported in ESM (source). So if there was an expectation that the file extensions are included in the source files, we wouldn't need this code (.ts vs. .js notwithstanding).

[aduth]

Should we use path.join ? Not sure about the Windows compatibility of this slash.

[dmsnell]

yes, but also Node normalizes all paths so Windows compatibility isn’t an issue.

On Windows, both the forward slash (/) and backward slash () are accepted as path segment separators; however, the path methods only add backward slashes ().

https://nodejs.org/api/path.html#pathsep

[aduth]

Should we reuse SOURCE_EXTENSIONS ? Maybe what we have currently as SOURCE_EXTENSIONS should be a separate constant that uses the raw array of extension values.

[aduth]

Actually, I wonder if we should be using some internal property of ESBuild like resolveExtensions.

To that point, is it possible to tap into ESBuild's internal resolution behavior so we don't have to reimplement this?

[jsnajdr]

I rewrote the plugin to use esbuild's internal build.resolve function. In the onResolve callback, ask esbuild to resolve the import (preventing recursion with pluginData) and then only modify the returned path. By replacing the source extensions (.tsx and others) with the target extension, which is either .js or .mjs.

That simplifies the plugin a lot. I no longer need to stat physical files, think about caching etc.

[jsnajdr]

Should we reuse SOURCE_EXTENSIONS ?

Probably yes, but there are complications: the globs want a bracket expansion syntax without dots, {js,jsx}, and for my code it's more convenient to have an array with dots, [ '.js', '.jsx' ]. Etc.

Let's consider this after everything else in this PR is in perfect order 🙂

[aduth]

Is there enough likelihood of reuse that it'd be worth considering to cache this result?

[aduth]

I think we could be more efficient to call stat once for the path and reuse the fs.Stats result for checking if it's a file or directory. With this code, we'll call stat twice for directory imports.

[jsnajdr]

I see a couple other export * from './types';

I updated the wordcount one. The shortcode file has an .js extension, can't use the type syntax there. Let's do it when it's converted.

[jsnajdr]

I noticed the transpilation is ~30% slower than trunk

That sounds right, because there's the additional work of resolving every import on the filesystem. Before this PR we were copying the import paths verbatim. But now we do some work that only bundler has to do: actually find the dependency on the fs.

I'm starting to think that we should have two "build" modes.

This concern could be addressed if we implemented the package bundling from #73516:

1. build the package bundles in **/build-module. One file for each entrypoint, the output format is an ES module with import and export statements.
2. build the scripts and modules needed by WordPress at runtime:
   • creating scripts is a matter of assigning exports to window.wp[handle] and replacing imports with window.wp[handle]. Plus minification. That's all, all the remaining work has already been done in step 1, without duplication.
   • building script modules: that's only minification, everything else has been done.

The upside is that both steps do work that is useful, and there's no duplication/overlap between them.

For the former we can assume "less TS" and maybe less of these "correctness" issues.

For day to day work in VSCode and with npm run dev, I think we need the types, don't we? At least in VSCode, but VSCode probably builds the types internally in the language server, in memory. And also in the pre-commit hook that does typechecking.

We don't need typechecking for building any executable Gutenberg artifacts, that's true. The only place where we need build-types is for published NPM packages. But for day-to-day development work types are relevant, it sounds unplausible that we wouldn't need them.

[jsnajdr]

@manzoorwanijk @anomiex The fixed packages have been published to NPM under the next tag. Maybe you want to try them out in Jetpack? 🙂 There's hasn't been any truly external testing yet, so we can't rule out that the packages are in fact completely broken.

[manzoorwanijk]

Testing in Automattic/jetpack#46389

[manzoorwanijk]

No difference because the extension is still .js and the syntax is ESM without type being module.

attw is even worse

[jsnajdr]

@manzoorwanijk There is a misunderstanding: the Jetpack PR and the attw screenshots still use the latest version from NPM, for @wordpress/block-editor it's 15.9.0. But I published only the "development" next tag, with version 15.9.1-next.738bb1424.0. So, nothing has changed at all in the upgrade PR.

I'd like to do at least some external testing before we publish the new packages as stable latest. Would it be possible to use the next packages in the Jetpack PR? Can we tell Renovate somehow to use next?

[manzoorwanijk]

But I published only the "development" next tag, with version 15.9.1-next.738bb1424.0. So, nothing has changed at all in the upgrade PR.

Ah sorry, didn't notice that. I thought it was a regular upgrade.

I created a test PR manually to see how it work - Automattic/jetpack#46397

[manzoorwanijk]

Almost every package breaks with "moduleResolution": "nodenext" but works fine with "bundler"

jest-console having no default export is broken.

Types are still broken

https://arethetypeswrong.github.io/?p=%40wordpress%2Fblock-editor%4015.9.1-next.738bb1424.0

[jsnajdr]

jest-console having no default export is broken

Yes, the CJS version of jest-console is broken, importing internal CJS modules with default exports (import supportedMatchers from './supported-matchers';) doesn't work. The __toESM helper produces a double default: { default: { default: obj } }. I'm looking into this.

It's very likely that many other CJS modules are broken the same way.

[jsnajdr]

We're running into this issue: evanw/esbuild#2623

When a file like jest-console/src/index.js imports a default export from a local submodule:

import supportedMatchers from './supported-matchers';

Then the transpiled CJS files' require is broken. The object resulting from require('supported-matchers.cjs') is wrong. It has the { default: { default: matchers } shape instead of { default: matchers }.

The advice in the PR is to never use default exports. That's not great. I'm trying to figure out what else we can do.