Skip to content

chore: add Node Releases JSON to bundle - #5443

Merged
ovflowd merged 6 commits into
nodejs:mainfrom
HinataKah0:node-releases-bundle-size
Jun 26, 2023
Merged

chore: add Node Releases JSON to bundle#5443
ovflowd merged 6 commits into
nodejs:mainfrom
HinataKah0:node-releases-bundle-size

Conversation

@HinataKah0

@HinataKah0 HinataKah0 commented Jun 19, 2023

Copy link
Copy Markdown
Contributor

Result: #5443 (comment)

This PR will include Node releases data JSON in the bundle. Based on our experiment, we found that fetching the JSON file in Client Side harms UX badly in slow 3G network. Not to mention that the bundle size increment is tiny and it takes a very long time for it to grow and become harmful.

We added node-releases-data.json containing an empty array [] as placeholder. This file will be overwritten during Build time and we don't want to commit unnecessary changes. However, once a file exist in the repository, .gitignore won't work anymore. As a workaround, we will have a pre-commit hook to discard the changes.

We use husky for pre-commit hooks (we followed these steps).

Related to: #5438

@vercel

vercel Bot commented Jun 19, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ❌ Failed (Inspect) Mar 19, 2024 3:05pm
nodejs-org-stories ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2024 3:05pm

@vercel

vercel Bot commented Jun 19, 2023

Copy link
Copy Markdown

@HinataKah0 is attempting to deploy a commit to the OpenJS Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 19, 2023 12:42 Inactive
@HinataKah0

HinataKah0 commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

Can I get someone to approve the deployment (Not PR)?
So I want to test on the actual environment since I am not sure if the compression is done different locally, etc...

@HinataKah0
HinataKah0 marked this pull request as ready for review June 19, 2023 12:51
@HinataKah0
HinataKah0 requested a review from a team as a code owner June 19, 2023 12:51
@HinataKah0 HinataKah0 changed the title chore: add Node Releases JSON to bundle (DON'T MERGE) chore: add Node Releases JSON to bundle Jun 19, 2023
@ovflowd

ovflowd commented Jun 19, 2023

Copy link
Copy Markdown
Member

@HinataKah0 I just approved your deployment. Sorry for the delay!

@vercel
vercel Bot temporarily deployed to Preview – nodejs-org June 19, 2023 20:38 Inactive
@mikeesto

Copy link
Copy Markdown
Member

And can someone help me to try pulling my branch and check if the node-releases-data.json is actually ignored or not. Git is quite picky about it. 👀

It is being tracked atm. You should be able to do git rm --cached public/node-releases-data.json to remove the file from Git's index, and then commit the change

@ovflowd

ovflowd commented Jun 19, 2023

Copy link
Copy Markdown
Member

FYI IMHO you should actually do the following:

  1. Remove the file public/node-release-data.json from .gitignore
  2. Delete the file and recreate it only with this values [] (empty array)
  3. Add the file and commit
  4. Then git update-index --skip-worktree public/node-release-data.json

This will tell git to ignore future changes to this file.

@HinataKah0

HinataKah0 commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

Then git update-index --skip-worktree public/node-release-data.json

If I understand correctly, this is only available in local machine.
Then, for others the file is still tracked.

Edit:
I am trying to google how others are usually doing this...

@HinataKah0

Copy link
Copy Markdown
Contributor Author

Included in the bundle:
Screenshot 2023-06-20 at 8 31 00 PM

Client Side:
Screenshot 2023-06-20 at 8 33 03 PM

Bundle impact is very minimal (as expected). It doesn't show up in bundle analyzer as well. 🤣

I tried simulating slow 3G network. And Client Side rendering performs really bad.

@ovflowd

ovflowd commented Jun 20, 2023

Copy link
Copy Markdown
Member

If I understand correctly, this is only available in local machine.
Then, for others the file is still tracked.

Incorrect. The command above applies to the repository. (Doesn't matter if local, others or upstream) as soon as they pull this commit it will affect them.

@HinataKah0

HinataKah0 commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

Client Side:

Screen.Recording.2023-06-20.at.8.43.20.PM.mov

126kB (compressed) -> 441kB (uncompressed)

Included in the bundle:

Screen.Recording.2023-06-20.at.8.45.30.PM.mov

127kB (compressed) -> 435kB (uncompressed) (I can't explain why this is lower uncompressed)

I feel that it's far better to include the Node Releases data in the bundle for now...
Since there isn't any real issue with the bundle size I believe (I googled it and it seems that the rule of thumb is to have it less than 300kB compressed).

Comment thread public/node-releases-data.json Outdated
Comment thread providers/nodeReleasesProvider.tsx Outdated
Comment thread providers/nodeReleasesProvider.tsx Outdated
Comment thread providers/nodeReleasesProvider.tsx Outdated
Comment thread providers/nodeReleasesProvider.tsx
@HinataKah0

HinataKah0 commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

After fighting with Git for some time...

I tried both approaches described above and some other combinations but nothing work 😅
I tried searching online as well, it seems that git update-index --skip-worktree works locally but not remote (this)

I think we just don't want to upset TS, right?
This works, TS is not upset with it. And not to mention that the file is going to be generated during build time so, I believe this will be okay.
Caveat: just handle the case if it's undefined/null.

Edit:
We can also declare the NodeReleaseJSON type inside the .d.ts file, it looks neat.

What do you all think?

@ovflowd

ovflowd commented Jun 20, 2023

Copy link
Copy Markdown
Member

Edit:
We can also declare the NodeReleaseJSON type inside the .d.ts file, it looks neat.

Not really needed.

I tried both approaches described above and some other combinations but nothing work 😅
I tried searching online as well, it seems that git update-index --skip-worktree works locally but not remote (this)

Not sure what you're doing wrong. Also not sure what behaviour is happening for you. I also have no idea what exactly you're doing. It would be interesting if you could write the steps you're doing.

@ovflowd

ovflowd commented Jun 20, 2023

Copy link
Copy Markdown
Member

I think we just don't want to upset TS, right?
This works, TS is not upset with it. And not to mention that the file is going to be generated during build time so, I believe this will be okay.
Caveat: just handle the case if it's undefined/null.

Not sure what you're saying. The JSON file type is resolved implicitly by TypeScript itself, so no need to define types for the JSON. The Provider itself has a type, and if you pass the mapped data it should also be all good.

If that is still not enough you can create a type for an individual entry of the array and pass it within the callback nodeReleaseData.map((release: NodeReleaseSource) => ...)

@HinataKah0

HinataKah0 commented Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

Oops, I just typed it out without re-reading again. 😅
Let me clarify things.

So, I tried exactly these steps:
#5443 (comment)

  1. Deleted public/node-releases-data.json from .gitignore
  2. Deleted the local JSON file & recreated the JSON file again with empty array []
  3. Committed it fda3146
  4. Ran git update-index --skip-worktree public/node-releases-data.json

It works in my local (the JSON file is not tracked).
I tried cloning my fork again in another directory, but the JSON file is still tracked.

I tried the step suggested by Mike as well but it also doesn't work.
Did step 1-3 again + Added back the JSON file to .gitignore + Ran git rm --cached public/node-releases-data.json
The file is still tracked.

I've tried other combinations of steps as well but none is working. Not sure if it just happened to me or not. 😅

Then, I checked online, it seems that git update-index --skip-worktree only works locally.

I believe our concern of having the JSON file with empty array [] is because we don't want to upset TypeScript. I checked for another alternative: f9db6e7

@ovflowd

ovflowd commented Jun 21, 2023

Copy link
Copy Markdown
Member

I believe our concern of having the JSON file with empty array [] is because we don't want to upset TypeScript. I checked for another alternative: f9db6e7

What if after committing the file with the empty array you re-add the file to the gitignore?

@HinataKah0

This comment was marked as outdated.

@HinataKah0

HinataKah0 commented Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

The step suggested by Mike will delete the actual file in remote, see here (talking about this outdated comment above)

What if after committing the file with the empty array you re-add the file to the gitignore?

I tried it as well...
It is tracked if the file has been committed (although it is inside gitignore)

@ovflowd

ovflowd commented Jun 21, 2023

Copy link
Copy Markdown
Member

I tried it as well...
It is tracked if the file has been committed (although it is inside gitignore)

What about this: https://stackoverflow.com/a/26245961

@HinataKah0

Copy link
Copy Markdown
Contributor Author

@HinataKah0
HinataKah0 force-pushed the node-releases-bundle-size branch from 88a03a1 to 324cfd8 Compare June 21, 2023 16:30
@HinataKah0 HinataKah0 changed the title (DON'T MERGE) chore: add Node Releases JSON to bundle chore: add Node Releases JSON to bundle Jun 22, 2023
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 23, 2023 09:17 Inactive
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org June 23, 2023 09:39 Inactive
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 26, 2023 09:04 Inactive
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org June 26, 2023 16:07 Inactive
@ovflowd

ovflowd commented Jun 26, 2023

Copy link
Copy Markdown
Member

@HinataKah0 did you follow Husky getting started steps correctly? The package.json should have been updated with a prepare step

@ovflowd

ovflowd commented Jun 26, 2023

Copy link
Copy Markdown
Member

NVM, just saw it.

@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 26, 2023 16:10 Inactive
@HinataKah0

Copy link
Copy Markdown
Contributor Author

I followed these steps: https://typicode.github.io/husky/getting-started.html#automatic-recommended

@ovflowd

ovflowd commented Jun 26, 2023

Copy link
Copy Markdown
Member

FYI I've pushed some little changes to your branch to fix some things. (Like Husky)

@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 26, 2023 16:22 Inactive
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 26, 2023 16:25 Inactive

@ovflowd ovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Good stuff, @HinataKah0!

@ovflowd

ovflowd commented Jun 26, 2023

Copy link
Copy Markdown
Member

BTW nvm the PR failures, that's something else that I'm patching on main. (And then undo the nextFonts change on this PR)

@ovflowd
ovflowd force-pushed the node-releases-bundle-size branch from ccb387b to 396c317 Compare June 26, 2023 16:37
@vercel
vercel Bot temporarily deployed to Preview – nodejs-org-stories June 26, 2023 16:45 Inactive
@HinataKah0

HinataKah0 commented Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

Thanks!
I saw the changes about the types. I'll verify them tmr.
Edited (because this PR has been merged, I don't want to make noise):
I've verified the changes, it looks good! I just knew about git sparse as well, thanks! 👀

FYI, my original intention of keeping different types (NodeReleaseJSON and NodeRelease) was because:
In useFetchNodeReleases (now it is moved to be inside the Provider itself), we are populating the empty fields with empty string ''. So in NodeReleaseJSON type, some fields are not guaranteed to exist but in NodeRelease they are.

I am okay with the new abstractions (NodeRelease extends NodeReleaseSource). I think it's neater.

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.

4 participants