Skip to content

Commit

Permalink
fix(build): address issue with dynamic import and vite (#5399)
Browse files Browse the repository at this point in the history
When we added a script for building the modules in `internal/` with
Esbuild in #5276 we needed to make a change to the function that Stencil
uses at runtime to lazy-load components (in
`src/client/client-load-module.ts`). Prior to #5276 we had a dynamic
import statement which looked like so:

```ts
import(
  `./${bundleId}.entry.js${BUILD.hotModuleReplacement && hmrVersionId ? '?s-hmr=' + hmrVersionId : ''}`
)
```

This constructs a filepath to the module for a given Stencil component,
accounting for HMR versioning, and then imports the module. All well and
good, but unfortunately this dynamic import does not play well with
Esbuild. As described
[here](https://esbuild.github.io/api/#non-analyzable-imports) when
Esbuild is in 'bundle' mode and it encounters an `import()` _and_ the
imported path or identifier looks "analyzable" it will attempt to
resolve the corresponding file and incorporate it into the bundle.

This is not always what you want! In particular, in our situation the
leading `"./"` in the template literal we had in `client-load-module.ts`
caused Esbuild to consider the `import()` an "analyzable" import and it
then tried to resolve and bundle the import instead of just leaving the
dynamic import in the code (as Rollup does in this case).

This created an issue because at _compile time_ (i.e. when Stencil
itself is built) this import does not resolve to anything, so Esbuild
would essentially transform that line into an empty import. This caused
runtime issues because the side-effect of the dynamic import was no
longer happening, so the modules containing Stencil component classes
and so on were not longer being loaded in.

To get this working for #5276 we pulled out the `"./"` string as a
separate variable, changing the template literal so it looks something
like this:

```ts
const MODULE_IMPORT_PREFIX = './';

import(
  `${MODULE_IMPORT_PREFIX}${bundleId}.entry.js${BUILD.hotModuleReplacement && hmrVersionId ? '?s-hmr=' + hmrVersionId : ''}`
)
```

This causes Esbuild to conclude that the import is "non-analyzable",
which addresses the issue and causes both Rollup and Esbuild to emit
equivalent code for this snippet, where both retain the dynamic import,
allowing for the runtime module resolution that we want here.

_However_, this broke the ability to use Stencil with Vite, which will
complain about non-analyzable imports if it sees a dynamic import which
does _not_ begin with `"./"`. See #5389 for details.

So essentially we have a situation where the behavior of Rollup,
Esbuild, and Vite is incompatible. The solution is to figure out a way
for both the Esbuild and Rollup builds to emit code in this case which
retains the dynamic import _and_ retains the leading `"./"` in the
template literal.

This is accomplished by retaining the `${MODULE_IMPORT_PREFIX}` in the
template literal, so that Esbuild does not attempt to analyze and bundle
the import, and adding plugins to both the Rollup and Esbuild bundles to
transform the emitted code before it is written to disk.

fixes #5389
STENCIL-1181
  • Loading branch information
alicewriteswrongs authored Feb 23, 2024
1 parent 74d9f94 commit 8ebacae
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 3 deletions.
3 changes: 2 additions & 1 deletion scripts/bundles/internal-platform-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { BuildOptions } from '../utils/options';
import { writePkgJson } from '../utils/write-pkg-json';
import { aliasPlugin } from './plugins/alias-plugin';
import { reorderCoreStatementsPlugin } from './plugins/reorder-statements';
import { replacePlugin } from './plugins/replace-plugin';
import { loadModuleReplacePlugin, replacePlugin } from './plugins/replace-plugin';

export async function internalClient(opts: BuildOptions) {
const inputClientDir = join(opts.buildDir, 'client');
Expand Down Expand Up @@ -55,6 +55,7 @@ export async function internalClient(opts: BuildOptions) {
aliasPlugin(opts),
replacePlugin(opts),
reorderCoreStatementsPlugin(),
loadModuleReplacePlugin(),
],
};

Expand Down
20 changes: 20 additions & 0 deletions scripts/bundles/plugins/replace-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,23 @@ export function replacePlugin(opts: BuildOptions): Plugin {
replaceData[`process.binding('natives')`] = '';
return rollupReplace({ ...replaceData, preventAssignment: true });
}

/**
* We need to manually find-and-replace a bit of code in
* `client-load-module.ts` which has to be present in order to prevent Esbuild
* from analyzing / transforming the input by ensuring it does not start with
* `"./"`. However some _other_ bundlers will _not_ work with such an import if
* it _lacks_ a leading `"./"`, so we thus we have to do a little dance where
* we manually replace it here after it's been run through Rollup.
*
* @returns a rollup string replacement plugin
*/
export function loadModuleReplacePlugin(): Plugin {
return rollupReplace({
// this ensures that the strings are replaced even if they are not
// surrounded by whitespace
// see https://github.com/rollup/plugins/blob/master/packages/replace/README.md#delimiters
delimiters: ['', ''],
'${MODULE_IMPORT_PREFIX}': './',
});
}
31 changes: 30 additions & 1 deletion scripts/esbuild/internal-platform-client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { BuildOptions as ESBuildOptions } from 'esbuild';
import type { BuildOptions as ESBuildOptions, Plugin } from 'esbuild';
import { replace } from 'esbuild-plugin-replace';
import fs from 'fs-extra';
import glob from 'glob';
Expand Down Expand Up @@ -45,6 +45,9 @@ export async function getInternalClientBundle(opts: BuildOptions): Promise<ESBui
...getBaseEsbuildOptions(),
entryPoints: [join(inputClientDir, 'index.ts')],
format: 'esm',
// we do 'write: false' here because we write the build to disk in our
// `findAndReplaceLoadModule` plugin below
write: false,
outfile: join(outputInternalClientDir, 'index.js'),
platform: 'node',
external: clientExternal,
Expand All @@ -59,6 +62,7 @@ export async function getInternalClientBundle(opts: BuildOptions): Promise<ESBui
// we want to get the esm, not the cjs, since we're creating an esm
// bundle here
externalAlias('@stencil/core/mock-doc', '../../mock-doc/index.js'),
findAndReplaceLoadModule(),
],
};

Expand Down Expand Up @@ -96,6 +100,31 @@ export async function getInternalClientBundle(opts: BuildOptions): Promise<ESBui
return [internalClientBundle, internalClientPatchBrowserBundle];
}

/**
* We need to manually find-and-replace a bit of code in
* `client-load-module.ts` in order to prevent Esbuild from analyzing /
* transforming the input by ensuring it does not start with `"./"`. However
* some _other_ bundlers will _not_ work with such an import if it _lacks_ a
* leading `"./"`, so we thus we have to do a little dance where we manually
* replace it here after it's been run through Esbuild.
*
* @returns an Esbuild plugin
*/
export function findAndReplaceLoadModule(): Plugin {
return {
name: 'findAndReplaceLoadModule',
setup(build) {
build.onEnd(async (result) => {
for (const file of result.outputFiles!) {
const { path, text } = file;

await fs.writeFile(path, text.replace(/\${MODULE_IMPORT_PREFIX}/, './'));
}
});
},
};
}

async function copyPolyfills(opts: BuildOptions, outputInternalClientPolyfillsDir: string) {
const srcPolyfillsDir = join(opts.srcDir, 'client', 'polyfills');

Expand Down
11 changes: 10 additions & 1 deletion src/client/client-load-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,16 @@ export const cmpModules = /*@__PURE__*/ new Map<string, { [exportName: string]:
* the below, but instead retains a dynamic `import()` statement in the
* emitted code.
*
* See here for details https://esbuild.github.io/api/#glob
* See here for details https://esbuild.github.io/api/#non-analyzable-imports
*
* We need to do this in order to prevent Esbuild from analyzing / transforming
* the input. However some _other_ bundlers will _not_ work with such an import
* if it _lacks_ a leading `"./"`, so we thus we have to do a little dance
* where here in the source code it must be like this, so that an undesirable
* transformation that Esbuild would otherwise carry out doesn't occur, but we
* actually need to then manually edit the bundled Esbuild code later on to fix
* that. We do this with plugins in the Esbuild and Rollup bundles which
* include this file.
*/
const MODULE_IMPORT_PREFIX = './';

Expand Down

0 comments on commit 8ebacae

Please sign in to comment.