-
Notifications
You must be signed in to change notification settings - Fork 114
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
fix: middleware being invoked more than once in a phase #337
fix: middleware being invoked more than once in a phase #337
Conversation
🦋 Changeset detectedLatest commit: 554a0db The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 Prereleases are available for testing 🧪 @cloudflare/next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5377487171/npm-package-next-on-pages-337 @cloudflare/eslint-plugin-next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5377487171/npm-package-eslint-plugin-next-on-pages-337 |
/** Tracker for the middleware that have been invoked in a phase */ | ||
private middlewareInvoked: string[]; |
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.
should/cloud this be a flag instead?
as in
private middlewareInvoked: boolean;
and you set it to true
/false
as needed?
do we need to check/care that the middlewarePath
is the same? shouldn't we just prevent multiple middlewares from running regardless?
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.
mh... maybe if middlewarePath
is different we should the different once 🤔
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.
I wanted to do something that would allow multiple middlewares on different paths to still get invoked.
Will there ever be a case where there are multiple with different paths? Not sure, maybe not, but that's why I didn't just do a Boolean.
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.
It was a fix for the same middleware running multiple times, so I feel like considering the path is somewhat important.
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.
yes I was just asking, all of this feels a bit unclear to me in any case 😓
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.
Thanks a lot @james-elicx 🙂
This PR does the following:
In the build output config, there are sometimes two records created for middleware in the
none
phase, both of which can be hit and then accidentally lead to multiple invocations. This PR introduces an array that tracks which middleware paths have been invoked when running a phase. If it tries to use one that has already been invoked, it will skip doing so, fixing the issue of multiple invocations.