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

Fix builder-webpack 5 usage in ESM environment #18620

Closed

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jul 2, 2022

Issue:

What I did

In ESM environments, require isn't available. Furthermore, it is required to specify
file extensions while importing files.

This MR will fix the usage of builder-webpack 5 in ESM environments
(type: module is defined in package.json).

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

In ESM environments, require isn't available. Furthermore, it is required to specify
file extensions while importing files.

This MR will fix the usage of builder-webpack 5 in ESM environments
(type: module is defined in package.json).
@nx-cloud
Copy link

nx-cloud bot commented Jul 2, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0186b8f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic valentinpalkovic changed the title fix: Fix builder-webpack 5 usage in ESM environment DRAFT: fix: Fix builder-webpack 5 usage in ESM environment Jul 2, 2022
@@ -32,7 +34,7 @@ window.__STORYBOOK_CLIENT_API__ = new ClientApi({ storyStore: preview.storyStore

preview.initialize({ importFn, getProjectAnnotations });

if (module.hot) {
if (import.meta.webpackHot) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a backwards compatible change?

Copy link
Contributor Author

@valentinpalkovic valentinpalkovic Jul 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The folder structure indicates, that this template is exclusively used for the webpack 5 builder. As I have understood the docs, it is totally valid to use this syntax also for CommonJS environments (https://webpack.js.org/api/hot-module-replacement/). So if your question is about backwards-compatibility regarding CommonJS environments, then yes. It should work. Especially, because it is already used in the next line.

@ndelangen
Copy link
Member

@valentinpalkovic Let's discuss this, I think this type of experimental work should be done based on future/base branch!

@ndelangen ndelangen marked this pull request as draft July 6, 2022 22:11
@ndelangen ndelangen changed the title DRAFT: fix: Fix builder-webpack 5 usage in ESM environment Fix builder-webpack 5 usage in ESM environment Jul 6, 2022
@ndelangen
Copy link
Member

Besides snapshots needed to be updated, this is good to merge. Fantastic work @valentinpalkovic

@IanVS
Copy link
Member

IanVS commented Oct 5, 2022

This brings up an interesting question. A bunch of addons all have something like this:

if (module && module.hot && module.hot.decline) {
  module.hot.decline();
}

Should this be updated across the board to match the webpack5 and vite syntaxes? (they're different, unfortunately)

And maybe the addon-generator too?

@ozzyogkush
Copy link

It would help to upgrade the webpack version to ~5.74.0 - it adds a new feature

resolve.extensionAlias option which allows to alias extensions

  • This is useful when you are forced to add the .js extension to imports when the file really has a .ts extension (typescript + "type": "module")

https://github.com/webpack/webpack/releases/tag/v5.74.0

@ndelangen
Copy link
Member

ndelangen commented Jan 13, 2023

@valentinpalkovic I updated the PR for you, how do you feel about this PR? close to getting merged?

And what about @ozzyogkush's comment?

@tmeasday
Copy link
Member

Regarding your comment @IanVS

A bunch of addons all have something like this:

I don't think they should. This code is to help in addon development (if it even helps at all these days) and certainly shouldn't be shipped to users. I deleted that code from a bunch of monorepo addons.

@yannbf
Copy link
Member

yannbf commented Nov 24, 2023

Is this still relevant @valentinpalkovic ?

@valentinpalkovic
Copy link
Contributor Author

@yannbf Yes, as soon as I have some time I will try to get this in.

@ndelangen
Copy link
Member

@valentinpalkovic I think this PR isn't going to be very helpful anymore, considering the merge conflicts.
It's likely easier to redo this work on the 8-0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants