elmish / hmr

Hot Module Replacement for Elmish apps
https://elmish.github.io/hmr
Other
28 stars 9 forks source link

Recent updates do not work with SAFE Template #35

Closed olivercoad closed 2 years ago

olivercoad commented 2 years ago

How to fix if you come accross this error

If you're using the SAFE template, upgrade it to Webpack 5.

Alternatively, if you prefer to stack with Webpack 4 at the moment, you can pin the version of Fable.Elmish.Hmr in your paket.dependencies file:

Fable.Elmish.Hmr == 4.1

Then paket install

dotnet paket install

Description

Using version 4.2 or above of Fable.Elmish.HMR leads to an error during webpack build.

Repro code

With safe template v3.1.1

dotnet new safe
dotnet tool restore
dotnet paket update Fable.Elmish.HMR
dotnet run

There is also a repo linked in https://github.com/SAFE-Stack/SAFE-template/issues/481

client: ERROR in ./output/App.js 113:14
client: Module parse failed: Unexpected token (113:14)
client: File was processed with these loaders:
client:  * ../../node_modules/source-map-loader/dist/cjs.js
client: You may need an additional loader to handle the result of these loaders.
client: |     }
client: |     const hmrState = new FSharpRef(null);
client: >     if (import.meta.hot) {
client: |         window.Elmish_HMR_Count = ((window.Elmish_HMR_Count == null) ? 0 : (window.Elmish_HMR_Count + 1));
client: |         import.meta.hot.accept();
client: Child html-webpack-plugin for "index.html":
client:      1 asset
client:     Entrypoint undefined = index.html
client:     [../../node_modules/html-webpack-plugin/lib/loader.js!./index.html] /home/oliver/repos/safenew/node_modules/html-webpack-plugin/lib/loader.js!./index.html 703 bytes {0} [built]
client:     [../../node_modules/lodash/lodash.js] /home/oliver/repos/safenew/node_modules/lodash/lodash.js 531 KiB {0} [built]
client:     [../../node_modules/webpack/buildin/global.js] (webpack)/buildin/global.js 472 bytes {0} [built]
client:     [../../node_modules/webpack/buildin/module.js] (webpack)/buildin/module.js 497 bytes {0} [built]

Related information

MangelMaxime commented 2 years ago

From my point of view, the problem is not with Elmish.HMR but because SAFE Template not being up to date with latest JavaScript stuff. Meaning it doesn't understand the being syntax used.

There are 3 solutions:

  1. Upgrade to Webpack 5 instead of Webpack 4
  2. Upgrade the template to use Babel allowing Webpack to understand newer syntax of JavaScript
  3. Pin to an older version of Elmish.HMR

From all of this solution, solution 1 is the easiest/best choice. Elmish.HMR is being tested against Webpack 5 and here is the configuration file used https://github.com/elmish/hmr/blob/master/tests/webpack/webpack.config.js

olivercoad commented 2 years ago

There is a PR open to upgrade webpack.

I think this is an issue with Fable.Elmish.HMR; dropping support for Webpack 4 is a breaking change so should be a major version change. Even the elmish templates use Webpack 4 (I know they're very out of date but still).

Actually even if it was a major version change it wouldn't've helped because unfortunately the SAFE template and elmish templates don't even set major versions...

Would I be correct in guessing that most users of this package are probably currently using Webpack 4?

If so, a lot of people are going run into this as soon as they do a paket update. Is there anything that can be done to prevent or help others running into this issue in the future? Is it possible to make it work with the older syntax, or at least give a more useful error message?

MangelMaxime commented 2 years ago

The thing is that we didn't drop support for Webpack 4 as it depends on how people have it set up.

Webpack 5 is out since 1 year now, and most (all?) of the bundler now support ES6 syntax out of the box.

We can't do anything to improve the error message from this package because this is Webpack which generates an error explaining what is happening.

Is it possible to make it work with the older syntax, or at least give a more useful error message?

Support for the older syntax is still here for example Parcel is using the old syntax but because Parcel also know how read ES6 code it doesn't throw an error. The only way, I can think of supporting the old syntax without adding new syntax would be to have the user define a compiler directives and then branch the code depending on it. Which is not an ideal solution as right now using Elmish.HMR with a modern (1+ year old) bundler works out of the box. It would also mean having a lot of compiler directives included making the code really hard to maintain.

It is true that I could have release Elmish.HMR as a new major version, but I didn't think it was a breaking change because the old syntax is still here. Even, if I did that people would probably still have the problem because most people don't pin their dependencies version based on the major range. Now that several versions has been released in the wild since 1 month, I think this is probably too late to act on this for little benefit.

olivercoad commented 2 years ago

I see.

It's a tricky one. I agree, I don't think using compiler directives is ideal. The only other solution I can think of is using a new package name on nuget and archiving the old one. Which is also not ideal.

We can't do anything to improve the error message from this package because this is Webpack which generates an error explaining what is happening.

Okay, this might be a silly thought, but webpack does actually print the line of code with the syntax error.

[<Emit("(import.meta.hot /* if error see https://github.com/elmish/hmr/issues/35 */)")>]
MangelMaxime commented 2 years ago

So I unlisted the recent released version and release it under a new major version 5.0.0.

This version, include your comment proposition as it didn't seems to affect the functionalities.

And it also fix an issue with Program.toNavigable when a bundler is detected.

The only other solution I can think of is using a new package name on nuget and archiving the old one. Which is also not ideal.

It would increase the maintenance work by a lot and also could confuse the user.

Thank you for taking the time to discuss the problem with me and coming up with the comment idea :)

olivercoad commented 2 years ago

Thanks @MangelMaxime, you've handled this issue well 👍.

I'll edit the issue description to add clear instructions for anyone who finds it.

I just checked and the comment doesn't show up as the error message is now

client: ERROR in ./output/App.js 119:74
client: Module parse failed: Unexpected token (119:74)
client: File was processed with these loaders:
client:  * ../../node_modules/source-map-loader/dist/cjs.js
client: You may need an additional loader to handle the result of these loaders.
client: |         const current = current_2;
client: |         window.Elmish_HMR_Count = ((window.Elmish_HMR_Count == null) ? 0 : (window.Elmish_HMR_Count + 1));
client: >         Internal_tryRestoreState(hmrState, (current.tag === 1) ? (((import.meta.webpackHot).accept(), (import.meta.webpackHot).data)) : ((current.tag === 2) ? (((module.hot).accept(), (module.hot).data)) : ((import.meta.hot.accept(), import.meta.hot.data))));
client: |     }
client: |     const map = (tupledArg_1) => [tupledArg_1[0], Cmd_map((arg0) => (new Msg$1(0, arg0)), tupledArg_1[1])];
client: Child html-webpack-plugin for "index.html":
client:      1 asset
client:     Entrypoint undefined = index.html
client:     [../../node_modules/html-webpack-plugin/lib/loader.js!./index.html] /home/oliver/repos/srtest/node_modules/html-webpack-plugin/lib/loader.js!./index.html 703 bytes {0} [built]
client:     [../../node_modules/lodash/lodash.js] /home/oliver/repos/srtest/node_modules/lodash/lodash.js 531 KiB {0} [built]
client:     [../../node_mod...

I think the comment will have to be added to every emit that mentions import.meta as I guess which one shows up first depends on the project.