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
[feat] adapter module #2285
[feat] adapter module #2285
Changes from 22 commits
cbfd2cf
f94fbe8
11b99fc
8fde219
f9bff84
09d007c
a76784b
17a9bbe
409b239
09cf6a9
6f50f5b
b07e677
f069539
61741f1
4abc0a2
6cb5be2
8fd0c4c
e0602fc
e84a688
fa20353
88d541a
e1af57c
d44975b
9dff930
9ab01b5
15f6149
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.
I liked having this line because it separates what the adapter itself is doing vs what is being done by the code that it outputs
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 agree the separation is nice, but it looks awkward to have nested lists in the site. I don't remember Sapper or Svelte docs with this types of list, not visibly at least, and the styles doesn't look quite right. We can probably achieve better separation with headings and/or numbered list.
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 don't think all adapters call esbuild, do they? Or at least it seems like it shouldn't be required if you don't need bundling. E.g. there's a ticket where esbuild causes a failure if you're trying to use TypeScript reflection, so perhaps we'd want to make it an option in
adapter-node
to allow people to turn it offThere 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 don't see any other way to include the
../output/server/app.js
other than to have a build step that bundles it, exceptadapter-static
of course. I don't know aboutadapter-node
though, but it's probably the most flexible of all. Maybe @jthegedus have any thoughts on this?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.
adapter-node
didn't have bundling for a long time. It was only added in #1648. I have mixed feeling about making it more required because I was thinking we should go the other way and make it optional as mentioned in the above commentThere 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.
These bullets should be "Call", "Convert", and "Globally shim" because you'd read them as a continuation of "Within the
adapt
method, an adapter should". Leaving off the "s" would also make it consistent with the first bullets which don't have an "s". Though this is a moot point if we add back "Output code that" because in that case it reads better with the "s"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.
Totally missed those! Agreed it would read better with the "Output code that" part, it might be better to make this its own section too as it's getting long.