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

bug: latest v4.12.3 causes: "dynamic import cannot be analyzed by Vite." error with Storybook #5389

Closed
3 tasks done
gregorypratt opened this issue Feb 21, 2024 · 7 comments · Fixed by #5399
Closed
3 tasks done
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@gregorypratt
Copy link

Prerequisites

Stencil Version

4.12.3

Current Behavior

When running in a Storybook environment Stencil v4.12.3 produces a build error.

The above dynamic import cannot be analyzed by Vite.
See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.

Expected Behavior

The error should not appear.

System Info

Package.json modules and versions: 


"@storybook/addon-essentials": "7.6.17",
"@storybook/addon-interactions": "7.6.17",
"@storybook/addon-links": "7.6.17",
"@storybook/blocks": "7.6.17",
"@storybook/html": "7.6.17",
"@storybook/html-vite": "7.6.17",
"@storybook/test": "7.6.17",
"@types/jest": "^29.5.6",
"@types/node": "^16.18.11",
"jest": "^29.7.0",
"jest-cli": "^29.7.0",
"puppeteer": "^21.9.0",
"react": "18.2.0",
"react-dom": "18.2.0",
"storybook": "7.6.17"

Steps to Reproduce

Create a new StencilJS project using npm init stencil.

Add storybook using npx storybook@latest init.
Choose html and Vite as the project options.

Create a preview-head.html file and add the following lines:

<link rel="stylesheet" href="dist/test/test.css" />
<script type="module" src="dist/test/test.esm.js"></script>

test being your project name.

Add a my-component.stories.ts file to your component directory with the follow straightforward storybook config:

export default {
  title: 'Example/Button',
};

export const Default = {
  render: `<my-component first="Stencil" last="'Don't call me a framework' JS"></my-component>`,
};

npm run build to compile the Stencil code.

npm run storybook to run Storybook and the error will occur.

Code Reproduction URL

https://github.com/gregorypratt/stencil-issue

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Feb 21, 2024
@rwaskiewicz rwaskiewicz self-assigned this Feb 21, 2024
@rwaskiewicz
Copy link
Member

Hey @gregorypratt 👋

Thanks for the detailed bug report! I do see the warning appearing here locally. Before we move forward here, is there anything else that we should know about in the reproduction? Specifically, is there anything broken here in terms of functionality (other than seeing the warning from vite)?

@rwaskiewicz rwaskiewicz added Awaiting Reply This PR or Issue needs a reply from the original reporter. and removed triage labels Feb 21, 2024
@gregorypratt
Copy link
Author

No I don't believe there is anything broken in the browser, and Vite is not used to build the dist of the component. It also doesn't look like there is an issue building storybook. However it of course makes it difficult if there are any other problems that are output to the console.

FWIW I have narrowed it down to the latest release specifically 👍

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Feb 21, 2024
@rwaskiewicz
Copy link
Member

Thanks! I'll get this ingested for someone to take a look

@rwaskiewicz rwaskiewicz added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Feb 21, 2024
@rwaskiewicz rwaskiewicz removed their assignment Feb 21, 2024
alicewriteswrongs added a commit that referenced this issue Feb 22, 2024
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 dyanmic
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
alicewriteswrongs added a commit that referenced this issue Feb 22, 2024
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
@alicewriteswrongs
Copy link
Contributor

alicewriteswrongs commented Feb 22, 2024

Hey @gregorypratt I just opened a PR with a fix that I believe addresses the issue - if you get a minute would you mind trying it out and confirming that it fixes it for you?

You can install a dev build with the fix like so:

npm install @stencil/core@4.12.3-dev.1708639078.b1a7006

alicewriteswrongs added a commit that referenced this issue Feb 22, 2024
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
@gregorypratt
Copy link
Author

@alicewriteswrongs boom, looks good to me!

Super quick turn around thank you 🙇‍♂️

alicewriteswrongs added a commit that referenced this issue Feb 23, 2024
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
@alicewriteswrongs
Copy link
Contributor

Excellent — thanks for trying it out! I'll get that merged today and the fix will be in the next release of Stencil - we'll ping here when that comes out

alicewriteswrongs added a commit that referenced this issue Feb 23, 2024
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
github-merge-queue bot pushed a commit that referenced this issue Feb 23, 2024
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
@rwaskiewicz
Copy link
Member

rwaskiewicz commented Feb 26, 2024

The fix for this issue has been released as a part of today's Stencil v4.12.4 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants