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
fix: restore dynamic-import-polyfill #3434
fix: restore dynamic-import-polyfill #3434
Changes from 3 commits
e29788a
35f3305
d688ebe
9b1a312
6b9c782
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.
question: Is
script
anHTMLScriptElement
?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 was the exact code from the polyfill from google that was removed, so I would prefer to avoid touching it. If we want to improve it, we can do so in another PR.
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.
HTMLScriptElement
would be just only a compile time code additionWould not touch runtime code 🤔
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.
Let's discuss it in another PR 👍🏼
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.
question: What is
6
? It's a magic number in this context 🤔Should we extract it into a variable/constant?
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.
6 is the number of letters in
import
, this is the same code that was removed before so it is ok to add it back as it was.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.
Ah, so
'import'.length
would theoretically better here, but yeah, feel free to add it in a new PR or we could use non-squash-merge specially for this PR and use two commits 🤔For readability and understandment, I would much prefer to use
'import'.length
hereThere 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 think the context is enough here, if we add something in another PR maybe it could be a hint in the comment above like
'import'.length === 6
. I think this kind of patterns is not uncommon in the Vite code base thoughThere 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.
Honestly I don't think it's a magic number, as it's quite clear to see it's the length of
import
and giving the previous comment saying "rewrite the import".'import'.length
would do but it's runtime, which to me, is not worth to bring the extra memory and CPU cost for the tiny DX that is not commonly touched anyway. I think it's ok to leave it as-is