Skip to content

fix(node): Improve chunk flushing #1985

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

Merged
merged 23 commits into from
Jan 20, 2024
Merged

fix(node): Improve chunk flushing #1985

merged 23 commits into from
Jan 20, 2024

Conversation

ScriptedAlchemy
Copy link
Member

@ScriptedAlchemy ScriptedAlchemy commented Jan 17, 2024

Description

improve chunk flushing by using module wrappers to track usage and invocations

Better ssr printing of js assets by tracking them via wrapped module, works similar to next/dynamic hoc

Related Issue

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

@ScriptedAlchemy ScriptedAlchemy changed the base branch from main to improve-public-path- January 17, 2024 08:04
@ScriptedAlchemy ScriptedAlchemy changed the title feat(runtime): Support return of custom factory when onLoad hook returns a function fix(node): Improve chunk flushing Jan 18, 2024
@@ -21,11 +22,24 @@ export const performReload = (shouldReload: any) => {
}

Object.keys(req.cache).forEach((key) => {
delete req.cache[key];
Copy link
Contributor

@jonthomp jonthomp Jan 19, 2024

Choose a reason for hiding this comment

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

Was this intentionally left in? You can remove the if and regex below if so

Copy link
Member Author

Choose a reason for hiding this comment

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

commenting it out, just to make sure i dont actually need this (if my regex covers everything)

@@ -8,14 +8,20 @@ import {

class MyDocument extends Document {
static async getInitialProps(ctx) {
await revalidate().then((shouldUpdate) => {
if (shouldUpdate) {
ctx.res.writeHead(307, { Location: ctx.req.url });
Copy link
Contributor

@jonthomp jonthomp Jan 19, 2024

Choose a reason for hiding this comment

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

Have you found out why this is required? We have the same - I started going deep, trying to track it down but with this workaround staring me in the face, I couldn't throw too many hours at it for now

Copy link
Member Author

Choose a reason for hiding this comment

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

becuae next is already running, so if you clear require cache mid way though, its router is destroyed.

require cache clear really needs to happen before next entry points are required.

When it comes to federation, next.js is about the worst you money can buy.

Maybe when we introduce true HMR i can find a better solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I worked aorund this in the past by using a proxy on get initial props and spawn a child worker to run the render and then just return the req/res via the worker and not the host.

Copy link
Contributor

@jonthomp jonthomp Jan 21, 2024

Choose a reason for hiding this comment

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

Ah ok, I had been exploring how I could do something like that, but inside the next.js app. Doing it outside is a rather interesting idea indeed! We haven't needed to run a custom next.js server yet but for this I think we can get one in place. Thanks I'll give it a try!

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are using a custom server, you can easily clear the cache by running the "clear cache" command before it hits the next route handler middleware. You can achieve this by referring to the custom server http example and placing the "require(/next)" in the middleware of HTTP server instead of at the top of the file. By doing this, you can clean and re-require Next.js whenever necessary.

It is entirely doable, but not easy to make it out of the box for users, hence the double redirect hack haha.

If you look in the server dist folder, you can see if Next.js emits some basic HTTP dev server in dev mode. If so, we can patch it via a Webpack plugin by overwriting it or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ScriptedAlchemy, we're not currently and would ideally stay away from a custom server. I had a look at your suggestion. but I don't think you can move the require(next) because it's used to prepare the server and create the route handlers.

We have some unique limitations in what we can do in our project, but I'm sure there is a solution with module federation involved. While I continue my journey into the code and try to find a solution, we've had the ok to book some time to chat with you in more detail. Hopefully, that's ok with you? We'll have a look and stick something in calendly

Base automatically changed from improve-public-path- to main January 20, 2024 01:22
# Conflicts:
#	packages/node/global.d.ts
#	packages/node/src/utils/flush-chunks.ts
#	packages/node/src/utils/hot-reload.ts
@ScriptedAlchemy ScriptedAlchemy merged commit 73eb07e into main Jan 20, 2024
@ScriptedAlchemy ScriptedAlchemy deleted the feat/chunk-lush branch January 20, 2024 04:38
@jonthomp
Copy link
Contributor

@ScriptedAlchemy please could you publish a test version from main including these latest enhancements?

@zhoushaw zhoushaw mentioned this pull request Jan 22, 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.

4 participants