-
Notifications
You must be signed in to change notification settings - Fork 116
[After 12.0] Migrate to Vite #2370
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
base: main
Are you sure you want to change the base?
Conversation
@@ -17,7 +17,7 @@ Kedro-Viz can exist either as: | |||
- A standalone web app, which is [published to PyPI](https://pypi.org/project/kedro-viz/) and can be run as a Kedro plugin from the CLI | |||
- A React component, which is [published to npm](https://www.npmjs.com/package/@quantumblack/kedro-viz) and can be imported into a larger React application | |||
|
|||
To allow the Kedro-Viz web app to be used as a Kedro plugin, first the JavaScript app is compiled into a static build, then it is bundled with a simple Python server and [published to PyPI](https://pypi.org/project/kedro-viz/). | |||
To allow the Kedro-Viz web app to be used as a Kedro plugin, first the JavaScript app is compiled into a static dist, then it is bundled with a simple Python server and [published to PyPI](https://pypi.org/project/kedro-viz/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 [vale] reported by reviewdog 🐶
[Kedro-viz.sentencelength] Try to keep your sentence length to 30 words or fewer.
@@ -17,7 +17,7 @@ Kedro-Viz can exist either as: | |||
- A standalone web app, which is [published to PyPI](https://pypi.org/project/kedro-viz/) and can be run as a Kedro plugin from the CLI | |||
- A React component, which is [published to npm](https://www.npmjs.com/package/@quantumblack/kedro-viz) and can be imported into a larger React application | |||
|
|||
To allow the Kedro-Viz web app to be used as a Kedro plugin, first the JavaScript app is compiled into a static build, then it is bundled with a simple Python server and [published to PyPI](https://pypi.org/project/kedro-viz/). | |||
To allow the Kedro-Viz web app to be used as a Kedro plugin, first the JavaScript app is compiled into a static dist, then it is bundled with a simple Python server and [published to PyPI](https://pypi.org/project/kedro-viz/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Kedro-viz.words] Use instead of 'simple'.
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
@@ -19,6 +19,9 @@ | |||
["babel-plugin-transform-remove-imports", { "test": "\\.(scss)$" }] | |||
], | |||
"presets": ["@babel/preset-env", "@babel/preset-react"] | |||
}, | |||
"test": { | |||
"plugins": ["transform-import-meta"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now importing Web Workers in Vite using import.meta.url, but since import.meta isn't transformed by default, we need to use the transform-import-meta Babel plugin to handle it correctly. This ensures the Web Worker paths are resolved properly during the build.
@@ -25,6 +25,7 @@ cypress/downloads/ | |||
|
|||
# production | |||
/build/ | |||
/dist/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Vite, the build output is dist/
by default. You’ll see this change reflected in a few places.
@@ -0,0 +1,30 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved index.html to the project root, as Vite requires it to be there.
@@ -67,7 +67,7 @@ def create_api_app_from_project( | |||
if Path(_HTML_DIR).is_dir(): | |||
# The html is needed when kedro_viz is used in cli but not required when running | |||
# frontend e2e tests via Cypress | |||
app.mount("/static", StaticFiles(directory=_HTML_DIR / "static"), name="static") | |||
app.mount("/assets", StaticFiles(directory=_HTML_DIR / "assets"), name="assets") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Vite, all bundled JS, CSS, and worker files are output under assets/
instead of static/
.
*/ | ||
const layout = async (instance, state) => instance.graphNew(state); | ||
// Ensure layoutWorker is assigned | ||
const layoutWorker = preventWorkerQueues(worker, async (instance, state) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webworkers work differently with Vite. Hence had to make a few changes here
@@ -1,5 +1,5 @@ | |||
import React, { useEffect } from 'react'; | |||
import * as d3 from 'd3'; | |||
import { select } from 'd3-selection'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import * as d3 from 'd3';
wasn't allowed in Vite
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
I can confirm there were no ESM changes and the build will happen when the new release is triggered. I can test once again. But for this PR, ESM is fine. The code changes looks fine to me. I will test this locally once again tomorrow. Also will this PR be merged after run-status or before ? Thank you @rashidakanchwala |
after run-status actually. |
run status has now been merged to main FYI @rashidakanchwala |
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
@@ -1,4 +1,14 @@ | |||
.PHONY: package build | |||
.PHONY: package build run pytest e2e-tests lint format-fix format-check lint-check secret-scan security-scan strawberry-server version sign-off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.PHONY: package build run pytest e2e-tests lint format-fix format-check lint-check secret-scan security-scan strawberry-server version sign-off | |
.PHONY: package build run pytest e2e-tests lint format-fix format-check lint-check secret-scan security-scan version sign-off |
Superb work @rashidakanchwala. I tested it locally (Not as React component) and all works well. To test it as React component I deployed the latest Vite branch to npm as
After above change it should work as expected but I encounter 1st error.
After changing the css path, till this point I started getting Kedro-Viz UI, but unable to get flowchart. With below log error, but I checked with current Kedro-Viz version, it also throw same errors but it works. To Tested the workflow view as well, As soon as I clicked on workflow tab it throws an error Note: i think But as soon as I pass the @rashidakanchwala Let me know if you have any questions. |
Signed-off-by: rashidakanchwala <[email protected]>
could you pls try again @jitu5 , i added import React in all the jsx files. |
@@ -159,8 +159,7 @@ export const inputKeyToStateKeyMap = { | |||
|
|||
export const PACKAGE_FSSPEC = 'fsspec'; | |||
|
|||
export const KEDRO_VIZ_DOCS_URL = | |||
'https://docs.kedro.org/projects/kedro-viz/'; | |||
export const KEDRO_VIZ_DOCS_URL = 'https://docs.kedro.org/projects/kedro-viz/'; | |||
export const KEDRO_VIZ_PUBLISH_DOCS_URL = `${KEDRO_VIZ_DOCS_URL}share_kedro_viz.html`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR but these link with .html
wont work with the new mkdocs anymore after the release. I'll make a separate PR to update them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @rashidakanchwala for this massive change. Everything works fine when i run it locally with the new environment, all the animation and interaction int the flowchart seems to remain the same.
Current Error: How to test locally after you make changes:
Prepare the simple react app to test Kedro-Viz npm package
My finding till this point: Looks like the problem is in bundling the workers, in
We need to explore more if the above change is the right or not. |
@rashidakanchwala I tested Vite migration with changes from this PR #2449 in VSCode, and its not working with |
what happens if we remove type module from here - https://github.com/kedro-org/kedro-viz/pull/2370/files#diff-118392ec1b808eb03ee9b1e09e29bba42b017416142c84d4e53321713c1491afR6 ? |
Also as we discussed yesterday, we need to find a way to bundle the worker for library build too and https://github.com/kedro-org/kedro-viz/pull/2370/files#diff-118392ec1b808eb03ee9b1e09e29bba42b017416142c84d4e53321713c1491afR4 is vite specific way of instantiating worker. So not sure if it works with webpack. |
|
Description
Migrating from react-scripts to Vite for faster builds and modern tooling. Resolves #2297 , and #2013
Development Notes
While this PR touches 223 files, ~95% of changes are simply renaming .js files to .jsx.
Some complexities involved Web Worker integration with Vite and required a few changes.
I've tested all the NPM commands and everything works as expected.
Pending verifications
I’ve tried both, but would like to confirm the following with the code owners:
QA notes
Please verify:
Checklist
RELEASE.md
file