Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Node.js standalone mode + support for astro preview #5056
Node.js standalone mode + support for astro preview #5056
Changes from 5 commits
360ec9f
64d6439
178eef3
cd00b9b
69e32cb
166a428
86954fd
d2b849e
19e887e
316d91b
264323d
9956b82
054aad7
70072ec
02cdb83
5dde984
12d6566
e7a9bd9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
thinking more about this, if you default to
middleware
and make this option required, do you still need the major version?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 wouldn't be required if it had a default, no?
We could default to
middleware
. My thought was that these two modes are pretty significantly different and it didn't feel right to "prefer" one over the other. Kind of like Vercel serverless vs edge, just completely different things.As far as semver, given that this is an integration I didn't think it mattered as much to do a semver major change.
But I don't feel too strongly on either of these things.
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.
yea, agreed that because its an integration this doesn't matter as much / I'm okay with the plan for a major. And I agree it makes more sense as required, or as defaulting to standalone. Defaulting to
middleware
is probably the least natural of the 3...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.
Ok, I think we should do a major in that case and require the option. We can track usage in the future and if
standalone
is overwhelming (and once it becomes more stable) we can make it the default if we want to.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.
nice! This makes a lot of sense!
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.
Just checking this is intentional, because I think I've only ever heard "runtime" as an adjective "runtime environment, runtime system, runtime library". If this is a noun, then it's fine!
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've definitely heard it used as a noun. In fact, nodejs.org's
<meta description>
tag uses it as one! view-source:https://nodejs.org/en/