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

Migration to Vite #61

Merged
merged 16 commits into from
Nov 1, 2024
Merged

Migration to Vite #61

merged 16 commits into from
Nov 1, 2024

Conversation

palagdan
Copy link
Collaborator

@palagdan palagdan commented Aug 29, 2024

Resolves #51
Resolves #58

@palagdan
Copy link
Collaborator Author

palagdan commented Aug 29, 2024

@blcham

After updating the extension from .js to .jsx for all the components, I encountered an issue with loading a pipeline. Specifically, after initializing a graph, the node state is empty. I tried to investigate it using debugging, but I couldn't figure out the problem. Everything else is working fine with Vite, including static images.
Screenshot from 2024-08-29 14-28-51

I encountered an error with WebSockets in Dagre.jsx: ReferenceError: Buffer is not defined.
I investigated and found that this error occurs because the Buffer class is a global object in Node.js but not in browser environments. The solution is to use the nodePolyfills plugin, which provides browser-compatible versions of Node.js core modules and globals. This allows libraries designed for Node.js environments to function correctly in the browser. We can discuss the possibility of removing this plugin and WebSocket from the Dagree component, as I don't see a strong reason to keep it.

@palagdan palagdan force-pushed the 51-migrate-to-vite branch from 7b85a9b to 90d8d53 Compare August 29, 2024 14:35
@palagdan
Copy link
Collaborator Author

palagdan commented Aug 29, 2024

To Do After Migration to Vite

  1. Refactor Class Components to Functional Components
    Functional components are recommended for better performance and simplicity.

  2. Refactor main.css
    Refactor main.css into separate files

  3. Fix Script Execution Issues
    Fix problems related to script execution.

  4. Migrate from TreeBeard to MUI Material for Script Tree Generation
    TreeBeard is outdated and causing warnings. Switch to MUI Material for generating the script tree due to the following warnings:

    TreeBeard2: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.
    
    Warning: Transition2 uses the legacy childContextTypes API, which is no longer supported and will be removed in the next major release. Use React.createContext() instead.
    
    ...

public/config.js Outdated Show resolved Hide resolved
src/components/treebeard/ScriptsTree.jsx Show resolved Hide resolved
@palagdan palagdan force-pushed the 51-migrate-to-vite branch from 452097a to b7998f8 Compare August 29, 2024 19:02
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

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

i assume everything is working fine now in SPipes editor, so after
comments are resolved we can merge .

vite.config.js Outdated Show resolved Hide resolved
src/config/env.js Outdated Show resolved Hide resolved
.env.development Outdated Show resolved Hide resolved
.env.production Outdated Show resolved Hide resolved
@blcham blcham mentioned this pull request Aug 30, 2024
14 tasks
@palagdan palagdan force-pushed the 51-migrate-to-vite branch from 421064a to 5129fd7 Compare August 30, 2024 16:28
@palagdan
Copy link
Collaborator Author

@blcham
I fixed a bug related to Dagre state management. It is a magic to me how it worked before and why it stopped working after I changed the file extension from .js to .jsx (I am sure this issue was caused by this change).

I discovered that setState() is an asynchronous function but does not return a promise, making its behavior somewhat unpredictable. The only way to ensure state updates are processed before taking action is to use a callback function as the second parameter of setState(). Unfortunately, you cannot await setState() directly.

The problem was that setState() was called multiple times within processGraph() and addNodes(), leading to an empty state. Additionally, this.renderCytoscapeElement() should be called after processGraph() has updated the nodes and edges state. However, since setState() is asynchronous, it didn't update the state in time, resulting in renderCytoscapeElement() receiving empty nodes and edges arrays. This is why the pipeline wasn’t rendered as shown in the picture I sent earlier.

   Rest.getScript(this.state.file, this.state.transformation).then((response) => {
        this._processGraph(response);
        this.renderCytoscapeElement();
      });

@palagdan
Copy link
Collaborator Author

palagdan commented Aug 31, 2024

@blcham

I am completely stuck with this ticket. Yesterday, after fixing a bug that I described earlier, I attempted to test it in Docker (production mode). When I tried to load the scripts, the page wouldn't load due to a polyfill issue with process. The external library undici uses Node.js libraries that are not supported by the browser, which is causing this error:
Screenshot from 2024-08-31 17-29-45
The undici library tries to access a Node.js version with the process variable:
Screenshot from 2024-08-31 17-52-38
Since the browser does not support Node.js libraries, process is undefined, and Vite does not support it either. For reference, see: vitejs/vite#7384.

I attempted to use the vite-plugin-node-polyfills library, but it did not resolve the issue and instead introduced new problems, such as memory leaks during the build and strange conflicts:

11.73 error during build:
11.73 [commonjs--resolver] Could not load /app/node_modules/stream-browserify/web: ENOENT: no such file or directory, open '/app/node_modules/stream-browserify/web'
11.73 file: /app/node_modules/jsonld/lib/index.js
11.73 Error: Could not load /app/node_modules/stream-browserify/web: ENOENT: no such file or directory, open '/app/node_modules/stream-browserify/web'

I don't know how to resolve this, as there is limited information available, and nothing seems to work.

I find out that using vite.js gives a different result between dev and build environment, that is why it works totally fine in a dev mode.

I tried to define process myself with

import Process from "process";
globalThis.process = Process;

But It is also undefined in production:

Screenshot from 2024-08-31 19-14-50

@blcham
Copy link
Contributor

blcham commented Sep 18, 2024

  1. please rebase
  2. describe e.g. from package-lock file where does unidici library comes from and why it is there
  3. i remember that there is vite plugin to set process variables, maybe we can set what is needed, not sure if relevant -- https://dev.to/whchi/how-to-use-processenv-in-vite-ho9

@palagdan
Copy link
Collaborator Author

palagdan commented Sep 19, 2024

  1. this library comes from s-forms -> jsonld -> http-client -> undici
    |Screenshot from 2024-09-19 11-58-17

  2. It is not relevant because it is guide for using env variables in vite. The problem is that the process variable is not defined.
    The undici calls process.version...

@blcham
Copy link
Contributor

blcham commented Oct 1, 2024

@palagdan to summarize our meeting:

  • the issue is happening only in production but not in npm run dev

Alternative solutions:

    1. fix it
    1. find out other way to make docker image (currently we use just nginx image to which we copy html+js files, maybe using Nodejs?)
    1. make docker image with npm that will run npm run dev

ad i):

  • how SForms causes the issue?
    • It is strange that it is caused by s-forms (s-forms -> jsonld -> http-client -> undici) although this error does not happen in Record Manager. What is different?
    • TODO - can we find out what exactly causes it? (debug using npm run dev)

@palagdan
Copy link
Collaborator Author

@blcham

I’ve fixed the bug, and it now works correctly in production mode as well. It’s ready for merging.

@palagdan palagdan requested a review from blcham October 29, 2024 15:44
@blcham blcham force-pushed the 51-migrate-to-vite branch from 5e9f8ad to 58c58d8 Compare November 1, 2024 10:59
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

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

LGTM

@blcham blcham merged commit 01c4cc8 into master Nov 1, 2024
1 check passed
@blcham blcham deleted the 51-migrate-to-vite branch November 1, 2024 11:05
@@ -58,10 +57,6 @@ const rankDirOptions = [
{ text: "TopBottom", key: "TB", value: "TB" },
];

const websocketURL = new URL("/rest/notifications", window.location.href);
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we removed websockets? Do we still support the automatic detection of changes within scritps?

Copy link
Contributor

@blcham blcham Nov 2, 2024

Choose a reason for hiding this comment

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

I just tested it and this does not work; please make a new issue for that.
To reproduce:

  • open a script
  • modify the script using a textual editor outside of SPipes editor

Copy link
Contributor

Choose a reason for hiding this comment

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

});
});
}

//prevent session timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we do not need this anymore?

@@ -96,16 +95,6 @@ class ScriptOntologyModal extends React.Component {
</td>
<td>{data[MODULE_EXECUTION_DURATION]}ms</td>
{/*<td><Moment unix format="DD.MM.YYYY">{data[MODULE_EXECUTION_START_DATE]/1000}</Moment></td>*/}
<td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still show execution start and finish date?

Copy link
Contributor

@blcham blcham Nov 2, 2024

Choose a reason for hiding this comment

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

BTW, execution is not working which you know i assume as we targeted it in #62 (not described there in detail so not sure)

@blcham
Copy link
Contributor

blcham commented Nov 2, 2024

@palagdan We still need to answer the comments above which I included into the first bullet of #62

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.

Icons for Script Modules do not load when starting the application locally. Migrate Webpack to Vite
2 participants