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

4575 upgrade vite 3.2.3 #4576

Merged
merged 7 commits into from
Nov 16, 2022
Merged

4575 upgrade vite 3.2.3 #4576

merged 7 commits into from
Nov 16, 2022

Conversation

eladlachmi
Copy link
Contributor

Upgraded Vite to v3.2.3 + updated plugins as required/recommended.
I've run through all the scenarios I can think of in dev mode, and everything looks to be working as expected.
If you have any thoughts on things I should try, please let me know.

I also fixed something in the loading of RepositoryObjectsPage. It's nearly unnoticeable when running in prod mode, but has been annoying me for a while with HMR in dev.

Resolves #4575

@eladlachmi eladlachmi added the include-changelog PR description should be included in next release changelog label Nov 8, 2022
@eladlachmi eladlachmi self-assigned this Nov 8, 2022
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

One comment about the DNS and looks like the current node crashes with the new code :/

import eslintPlugin from 'vite-plugin-eslint';
import replace from '@rollup/plugin-replace';
import dns from "dns";

dns.setDefaultResultOrder('verbatim');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need this one - the first request is the minimum nodejs version it requires and the fact that I don't have a problem dns resolved 'localhost' on local development.
Did you had an issue with the default dns resolution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder I didn't have an issue either until the Vite v3 upgrade. Without this Vite mounts on 127.0.0.1 and does not resolve localhost correctly (at least on my machine - node 16.16.0.) Come to think of it, while checking this, I also fixed the default port (back to 3000), so I'll double-check the need for modifying the DNS resolution locally again to be sure it's needed.

I see that the GitHub agent is running node 16.17.1. I'll nvm that locally and see if it's a node version issue or a GitHub agent memory issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, probably a good time to add a .nvmrc file to declare the preferred node version

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I read from the docs the issue is in case your dns localhost resolve don't not point to 127.0.0.1 the request may go to a different server - think it is a rare case and I am fan of using defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm is a specific tool - prefer to specify it in package.json using:

 "engines" : { 
    "node" : ">=16.0.0"
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder Fixed + tests are now passing

Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

LGTM

@eladlachmi eladlachmi requested a review from nopcoder November 9, 2022 12:57
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

We need to add --max-old-space-size=4096 to the relevant code gen step in the Makefile in order not to communicate it on every workflow and slack.

@nopcoder
Copy link
Contributor

nopcoder commented Nov 9, 2022

@eladlachmi one more thing related to the memory issue node have with vite:

  1. Not crashing with my node v18 - so maybe there is a fix for the 16.x branch too
  2. Check the option to disable the map files

@eladlachmi
Copy link
Contributor Author

We need to add --max-old-space-size=4096 to the relevant code gen step in the Makefile in order not to communicate it on every workflow and slack.

@nopcoder I think I'll take this even one step further and add it to the npm script for build
IMHO it's closest to the "source" of the issue

@eladlachmi
Copy link
Contributor Author

Removed sourcemaps from the production build. They aren't served anyway and just make the binary larger, since they are embedded. For the dev build I've set sourcemaps to inline, which improved the dev experience in Chrome devtools.

@eladlachmi eladlachmi requested a review from nopcoder November 10, 2022 16:09
@@ -6,7 +6,7 @@
},
"scripts": {
"dev": "vite",
"build": "vite build",
"build": "cross-env NODE_OPTIONS=--max-old-space-size=4096 vite build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - didn't used cross-env before (don't like extra packages). My thought was adding it to the Makefile so no specific value is forced into my build. or in my case when I'm trying 'bun' to build the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder I actually think the npm scripts is a great place to keep build-related node params. I think it's the right place for them. This is so inconsequential that I'm starting to feel a bit silly going on about it 😊

}
},
build: {
sourcemap: 'inline',
Copy link
Contributor

Choose a reason for hiding this comment

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

did you tried to turn it off to see if it remove the requirements for --max-old-space-size for the production build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder The default is off. This is just for the dev build.
Again, even if it does help, it's temporary. I don't see the point in avoiding it. You're just kicking the can down the road. I'd rather handle it now, while I'm in the context than in two months when we'll have to have the entire discussion again 😉

@eladlachmi eladlachmi requested a review from nopcoder November 13, 2022 10:45
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Approve in hope we will find a packaging tool based on non node.js soon.

@eladlachmi
Copy link
Contributor Author

Approve in hope we will find a packaging tool based on non node.js soon.

@nopcoder It's still a ways off, but this looks promising

@nopcoder nopcoder merged commit 2c8f8f8 into master Nov 16, 2022
@nopcoder nopcoder deleted the 4575-upgrade-vite-3.2.3 branch November 16, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Vite to v3.2.3
3 participants