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

build: Replace CRACO Build/Serve with Vite #5926

Merged
merged 27 commits into from
Feb 10, 2023
Merged

Conversation

ashtonG
Copy link
Contributor

@ashtonG ashtonG commented Feb 3, 2023

Description

This swaps out the utility used to run the dev server and build for the react webapp from craco to vite. Vite has a better track record of keeping up with its underlying dependencies than Craco/Create React App,, and it's build times and dev server startup times are shorter.

Test Plan

all app functionality should be unchanged.

Commentary (optional)

Changes to the source code that were made in order to get vite running and building:

  • Monaco syntax highlighters and plugins are now directly imported in the codebase rather than managed by plugin
  • stray require calls are removed
  • Notebookjs is now wrapped in a module and imported as a node package
  • Custom plugin that fixes a faulty import in the generated swagger api

Checklist

  • Changes have been manually QA'd

Ticket

WEB-815

@cla-bot cla-bot bot added the cla-signed label Feb 3, 2023
@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit b65ed36
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/63e3e9dbedc6bd000806e3b0
😎 Deploy Preview https://deploy-preview-5926--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ashtonG ashtonG changed the title Build: Replace CRACO Build/Serve with Vite build: Replace CRACO Build/Serve with Vite Feb 3, 2023
@ashtonG ashtonG marked this pull request as ready for review February 6, 2023 19:56
@ashtonG ashtonG requested review from a team and keita-determined February 6, 2023 21:42
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

Thats a cool change. Vite is pretty fast, but i found some issues.

  • i couldnt launch jupyter with This 127.0.0.1 page can’t be found error.
  • Every time i run npm i, packages-lock is modified. npm version is 9.3.1

also, i want somebody else from @determined-ai/web to review as well since its a significant change

webui/react/package.json Show resolved Hide resolved
webui/react/src/shared/configs/tsconfig.json Show resolved Hide resolved
webui/react/package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,63 @@
import crypto from 'crypto';
Copy link
Contributor

Choose a reason for hiding this comment

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

im not very familiar with csp code. Could you take a look at this, @mapmeld as an craco CSP author?

@ashtonG
Copy link
Contributor Author

ashtonG commented Feb 7, 2023

@keita-determined I'm unable to reproduce either issue locally or on the netlify build. Can you please verify your setup?

@keita-determined
Copy link
Contributor

@keita-determined I'm unable to reproduce either issue locally or on the netlify build. Can you please verify your setup?

  • i couldnt launch jupyter with This 127.0.0.1 page can’t be found error.
  • Every time i run npm i, packages-lock is modified. npm version is 9.3.1
    • I still get this one. I just ran the npm i. node version is v16.16.0 , npm version is 9.3.1
  • Also, make live opens http://127.0.0.1:3000, but preferably localhost:3000 is ideal.

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

Overall looked great! Sprinkled comments throughout based on the changes, but I'll run this branch once I have everything setup properly on my machine to actually run these changes.

webui/react/src/shared/utils/datetime.ts Outdated Show resolved Hide resolved
webui/react/src/shared/utils/datetime.ts Outdated Show resolved Hide resolved
webui/react/src/shared/utils/dom.ts Outdated Show resolved Hide resolved
webui/react/.eslintignore Outdated Show resolved Hide resolved
webui/react/Makefile Show resolved Hide resolved
@hkang1
Copy link
Contributor

hkang1 commented Feb 7, 2023

Tested:

  • make live - works as expected, but noticed that I generally had to refresh a couple of times before it took me to the login page? Jupyterlab does not run as it can't resolve via localhost:. Not sure if this ever ran properly via webdevserver though.
  • make build - works as expected locally (via devclusteer), jupyterlab runs perfectly
  • make test - still on craco (should this be migrated too, to vitest?)

@ashtonG
Copy link
Contributor Author

ashtonG commented Feb 8, 2023

@hkang1 when you say jupyterlab doesn't work, do you mean that it's stuck on queued or that the interactive page serves a 404? I just pushed a change that seems to prevent the popup url from being malformed. Can you try it out?

@keita-determined i think this might be the same issue you pointed out earlier.

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

All the comments and questions I had have been addressed. Conditional approval pending other folks' inquiries and comments.

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

overall LGTM too

few notes:

  • make live opens http://127.0.0.1:3000/det instead of localhost. According to this, its intended, so i think its good with http://127.0.0.1:3000/det. if there is an issue with this, we can change it in the future.

  • I still get the packages-lock.json change after npm i with node v16.16.0 and npm 9.3.1, but i can easily revert this change, so its ok.
Diff I got
diff --git a/webui/react/package-lock.json b/webui/react/package-lock.json
index 47ae0321f..e95e161f3 100644
--- a/webui/react/package-lock.json
+++ b/webui/react/package-lock.json
@@ -17999,8 +17999,13 @@
       }
     },
     "node_modules/notebook": {
-      "resolved": "src/vendor/notebook",
-      "link": true
+      "version": "0.7.0",
+      "resolved": "file:src/vendor/notebook",
+      "dependencies": {
+        "ansi_up": "^5.1.0",
+        "dompurify": "^2.4.0",
+        "marked": "^4.1.1"
+      }
     },
     "node_modules/npm-force-resolutions": {
       "version": "0.0.10",
@@ -28572,14 +28577,6 @@
         "type": "github",
         "url": "https://github.com/sponsors/wooorm"
       }
-    },
-    "src/vendor/notebook": {
-      "version": "0.7.0",
-      "dependencies": {
-        "ansi_up": "^5.1.0",
-        "dompurify": "^2.4.0",
-        "marked": "^4.1.1"
-      }
     }
   },
   "dependencies": {
@@ -40950,7 +40947,7 @@
       }
     },
     "notebook": {
-      "version": "file:src/vendor/notebook",
+      "version": "0.7.0",
       "requires": {
         "ansi_up": "^5.1.0",
         "dompurify": "^2.4.0",

nit: npm run analyze is not working on my local. i wonder how many ppl use this command
this issue can be fixed in diff PR

@ashtonG
Copy link
Contributor Author

ashtonG commented Feb 10, 2023

thanks, @keita-determined! i did a little digging and found this issue with nodejs/npm: nodejs/node#46542 -- this seems to be affecting us because we now install notebookjs as a local package instead of importing it directly. I think this has been fixed in later versions of npm as I'm unable to reproduce on npm 9.4.2. In addition, checking in the package-lock.json from 9.3.1 wouldn't be the right solution because our ci builds use the version of npm that ships with the version of node we use, and using the new lockfiles with the old versions causes npm to throw an error.

@ashtonG ashtonG merged commit 186033a into master Feb 10, 2023
@ashtonG ashtonG deleted the thiago-ashton/WEB-815 branch February 10, 2023 16:25
@dannysauer dannysauer added this to the 0.20.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants