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.

Sign up for GitHub

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

Jump to bottom

fix: OPTIC-1125: Label config preview always showing a step behind when making updates #6417

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: OPTIC-1125: Label config preview always showing a step behind when making updates #6417

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cce3ea3
fix: OPTIC-1125: Label config preview always showing a step behind wh…
bmartel Sep 20, 2024
b43b2e5
Adding comment about lazy load of LSF dependencies
bmartel Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 19 additions & 31 deletions web/apps/labelstudio/src/pages/CreateProject/Config/Preview.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,21 @@ import { useAPI } from "../../../providers/ApiProvider";

const configClass = cn("configure");

const loadDependencies = async () => import("@humansignal/editor");
let dependencies;
const loadDependencies = async () => {
if (!dependencies) {
dependencies = import("@humansignal/editor");
}
return dependencies;
};

export const Preview = ({ config, data, error, loading, project }) => {
loadDependencies();
bmartel marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

this way of calling it doesn't feel correct... besides of the comment from ellipsis it's also called synchronously.
or I'm just lacking some knowledge of dynamic import(). if you call it repeatedly and in parallel would the module be imported only once and the same instance will be returned in different places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you call it repeatedly and in parallel would the module be imported only once and the same instance will be returned in different places?

Technically there is no parallel capability but the dynamic import will happen only once, right on the first render cycle of the component. That starts the async load, which then is awaited in the useEffect to ensure it completes. It all references the same promise, and only assigns it once.

This is basically the loader pattern introduced with React 18, and Suspense, but without all that jazz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in DM, we can go with a different approach if you are feeling this is not resonating.


const lsf = useRef(null);
const resolvingEditor = useMemo(loadDependencies);
const rootRef = useRef();
const projectRef = useRef(project);
const api = useAPI();

const projectRef = useRef(project);
projectRef.current = project;

const currentTask = useMemo(() => {
Expand Down Expand Up @@ -53,12 +59,12 @@ export const Preview = ({ config, data, error, loading, project }) => {
}, [config]);

const initLabelStudio = useCallback(async (config, task) => {
if (!task.data) return;
await loadDependencies();

await resolvingEditor;
if (lsf.current || !task.data) return;

try {
const lsf = new window.LabelStudio(rootRef.current, {
lsf.current = new window.LabelStudio(rootRef.current, {
config,
task,
interfaces: ["side-column"],
Expand All @@ -82,12 +88,9 @@ export const Preview = ({ config, data, error, loading, project }) => {
},
});

lsf.on("presignUrlForProject", onPresignUrlForProject);

return lsf;
lsf.current.on("presignUrlForProject", onPresignUrlForProject);
} catch (err) {
console.error(err);
return null;
}
}, []);

Expand All @@ -99,24 +102,12 @@ export const Preview = ({ config, data, error, loading, project }) => {
}, [loading, error]);

useEffect(() => {
if (!lsf.current) {
initLabelStudio(currentConfig, currentTask).then((ls) => {
lsf.current = ls;
});
}
}, [currentConfig, currentTask]);
initLabelStudio(currentConfig, currentTask);

useEffect(() => {
if (lsf.current?.store) {
lsf.current.store.assignConfig(currentConfig);
console.log("LSF config updated");
}
}, [currentConfig]);

useEffect(() => {
if (lsf.current?.store) {
const store = lsf.current.store;

store.assignConfig(currentConfig);
store.resetState();
store.assignTask(currentTask);
store.initializeStore(currentTask);
Expand All @@ -126,18 +117,15 @@ export const Preview = ({ config, data, error, loading, project }) => {
});

store.annotationStore.selectAnnotation(c.id);
console.log("LSF task updated");
console.log("LSF updated");
}
}, [currentTask]);
}, [currentConfig, currentTask]);

useEffect(() => {
return () => {
if (lsf.current) {
console.info("Destroying LSF");
// there can be weird error from LSF, but we can just skip it for now
try {
lsf.current.destroy();
} catch (e) {}
lsf.current.destroy();
lsf.current = null;
}
};
Expand Down
Loading