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

Refresh preview on fast refresh #401

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

jakub-gonet
Copy link
Member

@jakub-gonet jakub-gonet commented Jun 21, 2024

Fast refresh is triggered when props are changed in previewed component. We didn't accommodate for that previously, as we only rendered preview once, after clicking a button from code lenses.

Additionally:

  • cleared up logic in code lenses and added a check for preview import to prevent false positives.
  • added check preventing crash on calling preview() without any component.
  • fixed URL being out of sync when switching between preview/non-preview

Fixes #288.

Copy link

vercel bot commented Jun 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 4, 2024 8:46am

@@ -209,6 +216,10 @@ export function PreviewAppWrapper({ children, ...rest }) {
};
LoadingView.hide = () => {
devtoolsAgent._bridge.send("RNIDE_fastRefreshComplete");
if (isRunningPreview()) {
// refresh preview component
openPreview(getCurrentSceneName());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like all the other changes in this PR except for this one. It doesn't seem like the right choice to manually fix fast-refresh behavior. I wonder why does it in the main app but not in preview, and would expect it to "just work" here as well? Maybe it's because preview is the root element and fast refresh doesn't support it? Maybe it'd be enough to just wrap it in some other root component?

onClick={() => project.goHome()}
onClick={() => {
project.goHome();
setUrlList([]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't ideal but we relied on metro reload to do it for us. In the future, we need to keep / always in the URL list. I'll create a ticket for that.

@@ -1,18 +1,21 @@
import { useSyncExternalStore, useEffect } from "react";
import { useRouter } from "expo-router";
import { store } from "expo-router/build/global-state/router-store.js";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the store import to the top so it's more evident that those two files are different only there.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments inline. Also please remove changes to navigation integration interface. If there are issues in expo router integration due to early navChange calls, this should be handled by that implementation alone. Also such changes shouldn't be a part of this PR as they seem to be fixing a different problem

const SceneTracker = require("react-native/Libraries/Utilities/SceneTracker");
const isRunningPreview = isPreviewUrl(SceneTracker.getActiveScene().name);
if (isRunningPreview) {
if (currentPreviewKey) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe check active scene name, we don't need the exact preview name here as we only check if its running or not

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please check the inline comment I added tho

}

export function preview(component) {
if (component._source == null) {
if (!component || component._source === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want to have == check here. I think _source could be undefined here too, and since it's internal field I wouldn't rely on it being set to null

@jakub-gonet jakub-gonet force-pushed the jgonet/refresh-preview-on-fast-refresh branch from ce8a8aa to 173cba8 Compare July 4, 2024 08:45
@jakub-gonet jakub-gonet merged commit 88f4caa into main Jul 4, 2024
3 checks passed
@jakub-gonet jakub-gonet deleted the jgonet/refresh-preview-on-fast-refresh branch July 4, 2024 09:19
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.

Preview is not working well when component changes.
2 participants