elmish / hmr

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

Elmish.HMR 3.0.2 doesn't work with Fable 2.x on RN #19

Closed forki closed 5 years ago

forki commented 5 years ago

screenshot_20190226-130050

Note: we don't use webpack here but React Native's own bundler. But this worked so far.

/cc @MangelMaxime

forki commented 5 years ago

It looks like the hot module is actually found, but it only has accept as single method defined. This is also described here: https://facebook.github.io/react-native/blog/2016/03/24/introducing-hot-reloading#hmr-api

I assume we should check if addDisposeHandler is defined an otherwise don't call it

MangelMaxime commented 5 years ago

If we don't have accept to addDisposeHandler, then it's a problem because we can't stop the old program.

We need to create a "polyfill" for it.

If we detect that addDisposeHandler doesn't exist, we could trigger a custom event here.

And then listen to the custom event [here)(https://github.com/elmish/hmr/blob/master/src/hmr.fs#L84-L88) if addDisposeHandler doesn't exist.

I would create two functions:

forki commented 5 years ago

that sounds like a good plan. would love to test it

MangelMaxime commented 5 years ago

that sounds like a good plan. would love to test it

Yes, but what I said is partially wrong 😅

hot.addDisposeHandler is called before the new application takes place because it's used to pass the oldState to the new application.

I am taking a look at the issue and will release a beta when ready.

MangelMaxime commented 5 years ago

I think I have a lead.

According to the master branch of metro hot should have these properties:

type HotModuleReloadingData = {|
  acceptCallback: ?HotModuleReloadingCallback,
  accept: (callback: HotModuleReloadingCallback) => void,
  disposeCallback: ?HotModuleReloadingCallback,
  dispose: (callback: HotModuleReloadingCallback) => void,
|};

But when I take a look at the file in my local project, I get:

type HotModuleReloadingData = {|
  acceptCallback: ?HotModuleReloadingAcceptFn,
  accept: (callback: HotModuleReloadingAcceptFn) => void,
|};

So I look at the blame history of the require.js polyfill and discover that since version v0.29+ they have implemented the dispose feature for HMR. Commit

Because metro is included as a dependency of react-native. I am upgrading SAFE Nightwatch to the latest version of react native. And will continue to work from there as I should now have access to dispose function which is an alias to addDisposeHandler.

Seems like I manage to upgrade to latest react native However, I needed to update the android/gradle project too. I will create a dedicated PR to SAFE Nightwatch so you can have a look at it.

forki commented 5 years ago

My production app already is on latest RN and still has this problem

Maxime Mangel notifications@github.com schrieb am Mi., 27. Feb. 2019, 12:37:

I think I have a lead.

According to the master https://github.com/facebook/metro/blob/master/packages/metro/src/lib/polyfills/require.js#L30-L35 branch of metro hot should have these properties:

type HotModuleReloadingData = {| acceptCallback: ?HotModuleReloadingCallback, accept: (callback: HotModuleReloadingCallback) => void, disposeCallback: ?HotModuleReloadingCallback, dispose: (callback: HotModuleReloadingCallback) => void,|};

But when I take a look at the file in my local project, I get:

type HotModuleReloadingData = {| acceptCallback: ?HotModuleReloadingAcceptFn, accept: (callback: HotModuleReloadingAcceptFn) => void,|};

So I look at the blame history of the require.js polyfill and discover that since version v0.29+ they have implemented the dispose feature for HMR. Commit https://github.com/facebook/metro/commit/edacc36cb41515db860f7fab6d4b455e6f4bf26c

Because metro is included as a dependency of react-native. I am upgrading SAFE Nightwatch to the latest version of react native. And will continue to work from there as I should now have access to dispose function which is an alias to addDisposeHandler.

Seems like I manage to upgrade to latest react native However, I needed to update the android/gradle project too. I will create a dedicated PR to SAFE Nightwatch so you can have a look at it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elmish/hmr/issues/19#issuecomment-467829475, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNNH6jO6WhTC0YlIPdFoy8XFd4GXoks5vRm31gaJpZM4bSCRi .

MangelMaxime commented 5 years ago

I didn't tested Elmish.HMR yet (time to eat^^).

But at least, I now see dispose handler.

capture d ecran 2019-02-27 a 12 50 54

And this is normal if you still have the error with the current version of Elmish.HMR it's using addDisposeHandler.

The things that suprise me is that in their definition file is that type HotModuleReloadingCallback = ()=> void; doesn't receive an argument. But I will see, if dispose is called as expected I can find a way to distribute the oldState in another way.

MangelMaxime commented 5 years ago

So it seems dispose is called when expected. I am working on a way to detect if the platform is ReactNative in order to pass the state using another mechanism because ReactNative doesn't give access to a data from dispose in order to pass information between instances.

MangelMaxime commented 5 years ago

HMR is working locally :)

The HMR is slow when recording the GIF but without the GIF recorder with are under the second.

2019-02-27 17 38 03

I will port the local code to Elmish.HMR this evening and will notify you when it's available as I also need to check I didn't break browser support.

forki commented 5 years ago

Wow. Awesomeness

Maxime Mangel notifications@github.com schrieb am Mi., 27. Feb. 2019, 17:40:

HMR is working locally :)

The HMR is slow when recording the GIF but without the GIF recorder with are under the second.

[image: 2019-02-27 17 38 03] https://user-images.githubusercontent.com/4760796/53506594-8f0d8e80-3ab6-11e9-8235-08abf9964677.gif

I will port the local code to Elmish.HMR this evening and will notify you when it's available as I also need to check I didn't break browser support.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elmish/hmr/issues/19#issuecomment-467936279, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNA1rt9w9RIGQev50K6Hyp_mZdkwiks5vRrUNgaJpZM4bSCRi .

MangelMaxime commented 5 years ago

Fix released in version 3.2.0

I am preparing the PR for Nightwatch. You will decide to merge it or no. :)

MangelMaxime commented 5 years ago

The fix will work only for the version of RN/Metro which implements the dispose function. This is a requirement to make the HMR works properly.

forki commented 5 years ago

It works like a charme and I think it's much faster with fable2 than before. This is amazing work! Thanks so much!!!