NativeScript icon indicating copy to clipboard operation
NativeScript copied to clipboard

fix(webpack): Method View._onLiveSync context is always undefined

Open CatchABus opened this issue 3 years ago • 1 comments
trafficstars

PR Checklist

  • [x] The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/CONTRIBUTING.md#commit-messages.
  • [x] There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
  • [x] You have signed the CLA.
  • [ ] All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/DevelopmentWorkflow.md#running-unit-tests-application.
  • [ ] Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/tools/notes/WritingUnitTests.md.

What is the current behavior?

Method View._onLiveSync argument is always undefined.

What is the new behavior?

Method View._onLiveSync argument is an object that implements ModuleContext interface as expected.

There is the need of context in a few places inside core modules. Few examples: https://github.com/NativeScript/NativeScript/blob/694146689b7729280aa99cd393848ede78d6cd37/packages/core/ui/core/view/view-common.ts#L220 https://github.com/NativeScript/NativeScript/blob/f70081d195320872e95d8031a2d807e7e47fcea0/packages/core/ui/frame/frame-common.ts#L625 https://github.com/NativeScript/NativeScript/blob/650ef59f9d4969d931ac6f36bdeaa42596295713/packages/core/application/index.ios.ts#L326

Fixes/Implements/Closes #9837 .

This is my proposal for solving the issue related. I took the liberty and replaced the old hot-loader approach of reading a file source and inject it in runtime, with a new function that contains code and the use of toString() call. This prints the source code of the function.

UPDATED: When hmr checks and retrieves modules that will apply changes to, a context argument will be set on __onLiveSync call.

There is also this case which runs if injectHMRRuntime is set to false: https://github.com/DimitrisRK/NativeScript/blob/8855ca43735f6e6f6ffd3598ff3ea2d18d57fbbd/packages/webpack5/src/loaders/nativescript-hot-loader/index.ts#L30 I wasn't sure if I should meddle with this code injection, so I left it intact.

@rigor789 Provided that we have found this patch okay or have applied all necessary changes, there might be a need for few changes in core modules too. Would it be alright to add those changes in the same PR?

CatchABus avatar Mar 22 '22 10:03 CatchABus

As mentioned in #6665, this mechanism was implemented for the following reasons:

Expose HmrContext interface.
Apply changes in app.css or app.scss instantly.
Avoid navigation on livesync when changes in app.css or app.scss have been made.
Apply changes in app.css or app.scss on back navigation.

Core contains dozens of ugly checks for such minor details. Perhaps, this doesn't need much attention for now as my solution will also need several tweaks inside core to work. On the other hand, the page history problem I encountered during live-sync (which is the reason I reported this) can be easily solved so we may focus on this one.

CatchABus avatar Apr 23 '22 23:04 CatchABus

I close this as it's a vanilla-specific feature that worked with webpack 4. We need features that all flavors can benefit from.

CatchABus avatar Mar 13 '23 22:03 CatchABus