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(astro): add Built-in SVG component support #12067
feat(astro): add Built-in SVG component support #12067
Changes from 13 commits
aa3f606
0832ae3
5f128ea
434ceda
f8e94e3
2b6744b
a751408
11e2eba
3c576fd
3f274c4
a92cb57
66eec7a
fcc6eca
78490fb
ddf5604
767559c
31b08e7
c03a467
6ab0f1a
d40d45d
9344a25
28d6c38
676add9
08e5708
6e41401
7b4a3a9
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.
IIUC, if this is on the module-level (and also globally as a singleton), wouldn't this value not be incremented on a per-page basis? If per page, I think it should be bound to the
result
fromcreateComponent
as a singleresult
is created when rendering per page. So maybe we can use a WeakMap here to track the ids perresult
, which we can also use it to track the first render instead of usingWeakSet<Response>
?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 that
WeakMap
seems like a good idea 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.
I'm not quite following how to use the
WeakMap
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.
The suggestion here is to use
const ids = new WeakMap<SSRResult, number>
instead to track theid
perresult
(fromcreateComponent
)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'm having trouble getting this to work. I'll give it a shot again tomorrow. Maybe I'm just tired 😅
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'll help you out :)
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.
Here
6ab0f1a
(#12067)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! I missed that we would still need to keep the counter/ids. I'm curious what the
WeakMap
adds in this case since we're still generating the ids by the counter.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.
We don't need the
counter
variable there. What I mean is that the WeakMap values are the counter itself that starts from 0 on first assigned.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'll give it a shot but I was having issues previously with 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.
Would love @Princesseuh's take on this. Any expected footguns with including the file content for SVGs in the image metadata?
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.
If you have a lot of SVGs, this can be problematic for SSR since it'll get in your bundle, that's my only concerns
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.
Right. Unfortunately that will be unavoidable for this feature.
I think the DX improvement is worth the tradeoff, though!
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.
This is definitely outside my realm of expertise. I'll defer to whatever the consensus is 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.
Something we could do one day (definitely outside of this PR), though it'd make SVG icons somewhat slower in SSR is add a lazy
data
property or something, that's essentiallyasync () => await fetch(this.src)
in SSR and something else in SSG so that you can load the data without it being added to the bundle.