Add HMR and React Fast Refresh support#12060
Conversation
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SchrodingersGat
added
plugin
|
This depends on #12058 being merged first - which adds more unit testing for plugin defined UI components |
|
@xhivo97 your code needs formatting - run |
|
I've added a check for where the module was loaded from, and only load the new module it if it's from the same path. This should fix some of the issues I've found so far. The original error handling hasn't been added yet, I can hopefully add that today. But that should be the last missing functionality the original had. Any recommended plugins I should test with this with? I don't know of any using the legacy entrypoint, so having one to test would be great. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12060 +/- ##
=======================================
Coverage 91.47% 91.47%
=======================================
Files 979 979
Lines 52331 52331
=======================================
Hits 47870 47870
Misses 4461 4461
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Below is the testing I have manually performed, with the following terms:
Tests
Observations
Legacy Plugins
I have manually tested with version 
And it works with compiled UI code too (note port 
|
|
Running tests again - the new playwright tests should cover plugin loading sufficiently |
@xhivo97 please ping me again when you've added this and I'll review once more |
|
@SchrodingersGat Thank you for the review, and especially for testing it. I'm almost done with the error handling but will continue tomorrow since it's a bit late here. |
|
Is changing the messages acceptable? I'm thinking maybe it's not allowed because of translations. So far I have these 4 cases handled, which should be all I think. Maybe these messages could be improved, as well. 
I also re-added After which, there's some very minor issues like having to manually reload the page if the exported function signature has changed, which are maybe not worth the effort of fixing. |
It's ok as long as we expose the strings for translation. I am in favor of providing better error feedback here.
Agreed, not worth the effort. |
| }); | ||
|
|
||
| return componentFn ? ( | ||
| <ApiProvider client={queryClient} api={api}> |
|
Hope the most recent commit is an improvement. Error handling, and |
|
@xhivo97 checked this again, still seems to work across compiled and live plugins, HMR still working nicely too! A couple of small requests: DocumentationPlease update the documentation around plugin development, especially this section: 
ChangelogPlease add an entry into |
xhivo97
force-pushed
the
enable-hmr-and-react-fast-refresh
branch
from
524e253 to
1f385ab
Compare
|
@SchrodingersGat I just updated the docs and I didn't find any more references to the changes in the docs, but it's possible I have missed some. |
|
I am still unsure that this change is OK: - <Boundary label={identifierString(`RemoteComponent-${exportName}`)}>
- <LanguageContext key={remountKey}>
- {componentFn(pluginContext)}
- </LanguageContext>
- </Boundary>
- ) : errorMsg ? (
- <Alert
- color='red'
- title={t`Error Loading Plugin Content`}
- icon={<IconExclamationCircle />}
- >
- <Text>{errorMsg}</Text>
- </Alert>
+ <ApiProvider client={queryClient} api={api}>
+ <MantineProvider
+ theme={context.theme}
+ defaultColorScheme={context.colorScheme}
+ >
+ <LanguageContext>{componentFn(context)}</LanguageContext>
+ </MantineProvider>
+ </ApiProvider>The rationale behind this was to remove the stuff that's supposed to be called only once. But the fact that this seems to break if you remove I don't really understand how all that should work, exactly. So if someone knows, I'd love an explanation. I also assume this is related to why ETA: I looked into this and think this is OK, so in addition I have removed |
|
with not creating a new root anymore these removals should be fine |
The incoming module needs to include the URL from which it was loaded, so that it's possible to enforce only loading modules imported from the same pathname as the current module.
- Add error handling to `useRemotePlugin` and simplify `RemoteComponent` - Improve HMR to use a registry instead of a single global callback. This should now handle two legacy plugin entry points being used at the same time via RemoteComponent.
LanguageContext should not be necessary here, as it's provided in ThemeContext, which is used in InvenTree's frontend entry.
xhivo97
force-pushed
the
enable-hmr-and-react-fast-refresh
branch
from
ef62f79 to
a5d44f9
Compare
|
While not proper fast refresh, if you comment out |
|
Looks like the broken panel plugin is not providing the expected error message (or any error message): 
Are you aware of how to download the playwright logs and dig into the errors? |
|
@SchrodingersGat I can look into this. Just briefly looked into the logs in the runner and could it be related to this: |
|
@SchrodingersGat It might be that it's looking for this: But that has changed and the test probably needs updating |
|
https://docs.inventree.org/en/stable/develop/react-frontend/#testing For how to setup testing locally |
3 participants
This enables React Fast Refresh in Vite's dev server.
It introduces
UseRemotePlugin.tsx, which provides hooks that factor the module loading and rendering logic out ofRemoteComponent.hmrSetModuleis only needed to load updated modules for legacy plugins. I'm not sure if they are even developed with Vite. If they are, then this, together with the Vite plugin, should reload the module on file saves, without triggering a full refresh.The plugin creator PR: inventree/plugin-creator#109