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

Feature/hmr parcel webpack vite #30

Merged
merged 15 commits into from
Nov 8, 2021

Conversation

MangelMaxime
Copy link
Member

@MangelMaxime MangelMaxime commented Nov 6, 2021

Status:

  • Webpack ✔️
  • Parcel ✔️
  • Vite ❌
  • Snowpack ❌

@alfonsogarciacaro I don't understand why Vite HMR is not triggering.

Webpack

Webpack is supported via import.meta.webpackHot and module.hot if the first one is not defined.

Parcel

Webpack is supported via module.hot

Vite

Vite should be supported via import.meta.hot and I do access it correctly when log it's value but the HMR is not working. It is always triggering a full reload.

I know that the current way I use to access the hot module is not standard:

[<Emit("import.meta.hot ? import.meta.hot : (import.meta.webpackHot ? import.meta.webpackHot : module.hot)")>]
let hot : IHot = jsNative

But even, when I tried to just write code specific to Vite to follow the "Required Conditional Guard".

[<Emit("import.meta.hot")>]
let hot : IHot = jsNative

Also, in our case we don't really need to add the guard because when bundling for production we don't generate HMR code thanks to compiler directives.

I was not able to make it works. Do I need to do something special when starting Vite?

Snowpack

I can't seems to configure Snowpack correctly... I always get an error like that:

[23:39:08] [snowpack] Package "react-native" not found. Have you installed it?

According to the documentation this happen when you don't import package using ./ or / in the path but I don't have any reference to react-native in my code so I don't know why it is asking for it.

If we don't have Snowpack working, this is not so much of a problem to me as the way Snowpack is working should be similar to Vite.


I didn't finish cleaning up the repo architecture but to run the tests project you can do:

  1. npm install

  2. npm run tests:watch

    This is going to start Fable and all the bundlers at the same time.

Than you can edit the App.fs file line 96 and see that:

  • The timer is not reset
  • That the display is stable, by that I mean that if we didn't dispose correctly of the previous App, you will have a strange behaviour where both instance of the application are flickering because they both render to the same div.

…er listener correctly + more bunlders

- Webpack: OK
- Parcel: OK
- Vite : KO
- Snowpack : KO
@alfonsogarciacaro
Copy link
Contributor

alfonsogarciacaro commented Nov 8, 2021

It's not documented but it seems Vite parses the file and looks for the if (import.meta.hot) pattern (see this code for example), that's why they require it and why I had to put the code to deal with Vite and Webpack in two different branches. In fact, at one point also the HMR bindings were not working for me because Fable was adding parentheses, so I had to make sure import.meta.XXX expressions were called without parentheses.

@MangelMaxime
Copy link
Member Author

Hum,
I am ranting but there choice is just ...

Who though it would be a good idea that the code will depends on how it is written and not how it executing...

The key point to make Vite works seems to be that:

In fact, at one point also the HMR bindings were not working for me because Fable was adding parentheses

For Vite (import.meta.hot).dispose is not the same as import.meta.hot.dispose.

In JavaScript (at execution) they are the same. It reminds me of React-Refresh etc. which ask you to write the code in a specific way... People complained in the past that they had to write code in a specific way for HMR to works, but they manage to make it even more specific 🤦

Thank you for your input, I will have to duplicate code like you did to make it works.

@MangelMaxime MangelMaxime force-pushed the feature/hmr_parcel_webpack_vite branch from 03ee827 to 33f2bf3 Compare November 8, 2021 17:12
@MangelMaxime
Copy link
Member Author

I now have Webpack, Vite and Parcel working with a not too much code duplication

Accept HMR

// Vite needs to be the first HMR tested
// because Vite is pretty stricit about how HMR is detected
if HMR.Vite.active then
    updateElmish_HMR_Count ()
    HMR.Vite.hot.accept()
    Internal.tryRestoreState hmrState HMR.Vite.hot.data

else if HMR.Webpack.active then
    updateElmish_HMR_Count ()
    HMR.Webpack.hot.accept()
    Internal.tryRestoreState hmrState HMR.Webpack.hot.data

else if HMR.Parcel.active then
    updateElmish_HMR_Count ()
    HMR.Parcel.hot.accept()
    Internal.tryRestoreState hmrState HMR.Parcel.hot.data

Stop the old program

// Vite needs to be the first HMR tested
// because Vite is pretty stricit about how HMR is detected
if HMR.Vite.active then
    HMR.Vite.hot.dispose(fun data ->
        Internal.saveState data !hmrState

        dispatch Stop
    )

else if HMR.Webpack.active then
    HMR.Webpack.hot.dispose(fun data ->
        Internal.saveState data !hmrState

        dispatch Stop
    )

else if HMR.Parcel.active then
    HMR.Parcel.hot.dispose(fun data ->
        Internal.saveState data !hmrState

        dispatch Stop
    )

Browser Navigation listener handling

// Vite needs to be the first HMR tested
// because Vite is pretty stricit about how HMR is detected
if HMR.Vite.active then
    HMR.Vite.hot.dispose(fun _ ->
        Navigation.Program.Internal.unsubscribe ()
    )

else if HMR.Webpack.active then
    HMR.Webpack.hot.dispose(fun _ ->
        Navigation.Program.Internal.unsubscribe ()
    )

else if HMR.Parcel.active then
    HMR.Parcel.hot.dispose(fun _ ->
        Navigation.Program.Internal.unsubscribe ()
    )

LazyView component

override this.shouldComponentUpdate(nextProps, _nextState) =
    // Vite needs to be the first HMR tested
    // because Vite is pretty stricit about how HMR is detected
    if HMR.Vite.active then
        this.shouldComponentUpdateHMR nextProps

    else if HMR.Webpack.active then
        this.shouldComponentUpdateHMR nextProps

    else if HMR.Parcel.active then
        this.shouldComponentUpdateHMR nextProps

    else
        not <| this.props.equal this.props.model nextProps.model

I am still unable to configure Snowpack because it is asking about react-native but hopefully it use the same API as one of this 3 bundlers.

I will do a pause before re-testing this PR and if everything is good, I will publish a new version.

@MangelMaxime MangelMaxime merged commit c36fedc into master Nov 8, 2021
@MangelMaxime MangelMaxime deleted the feature/hmr_parcel_webpack_vite branch December 22, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants