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
Astro Integration System #2820
Astro Integration System #2820
Changes from 1 commit
7efbe23
d9ad4f4
38bd8df
134d29b
42a2f02
1dd3030
f94af7a
4ffc259
659ad8d
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 don't know if I will die on this hill (I might) but I think mixing configuration and state is a mistake that will be hard to unravel. Probably the right thing to do is create a class/object that wraps the config and then pass that everywhere (I believe that is what ResolvedConfig is for in Vite).
But that would involve updating a crazy amount of code, so I understand not wanting to do that.
How about a compromise where this context object is kept in a global WeakMap?
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 with this. Not sure the best solution but I don't love shoving a stateful object in here.
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, I tried to leave the TODO to capture that need here. I agree, a
context
object isn't really a property of your config. If anything, it's the opposite, so maybeAstroContext
becomes the class that manages your config and other stuff.If we're talking whats in scope for this PR and the short-term, I think I prefer the
_ctx
hack vs. maintaining a weakmap separately. It's not a hill that I'll die on either, but I don't see that as any easier/harder to unravel than_ctx
.The compromise that I'd suggest instead would be to fast-track that larger Class/Object refactor instead of letting this sit for to long post-merge. If we could commit to that (maybe @bholmesdev could take a look next week?) then I think
_ctx
is the better short-term path.