fix(build): clean up of build-www rewriteImports (follow-up to #5995) #5999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz and
ivailop7
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
pieterv
reviewed
May 1, 2024
scripts/www/rewriteImports.js
Outdated
Comment on lines
54
to
63
| // This is mutated in-place because I couldn't find a mutation that | ||
| // did not fail for replacing the Program node. | ||
| node.docblock.comment.value = node.docblock.comment.value.replace( | ||
| / \* @flow strict/g, | ||
| ' * @flow strict\n * @generated\n * @oncall lexical_web_text_editor', | ||
| ); | ||
| // Let the transform know we actually did something. | ||
| // Could not figure out the right way to update the | ||
| // docblock without an in-place update | ||
| context.addLeadingComments(node, ''); | ||
| // We need the mutations array to be non-empty, so remove something | ||
| // that is not there. The AST traversals use object identity in a | ||
| // Set so we don't have to worry about some other line changing. | ||
| context.removeComments(t.LineComment({value: ''})); |
There was a problem hiding this comment.
Yeah sorry mutating the program node is not really supported atm, i have a task to fix it. Your solution looks roughly reasonable. The other maybe more correct (but still not ideal) solution is to find the doc block comment within the program. It will be attached to the first statement of the program, then you can use the standard comment remove and add API's.
acywatson
previously approved these changes
May 1, 2024
|
I'll take a look at that approach and write some unit tests |
…tement constraint
Closed
acywatson
approved these changes
May 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Follow-up to #5995, while this fix worked it was not as clean as it should be and probably shouldn't pass flow type checks if it was analyzed. I did a little more digging and I found a more correct incantation, but it could probably still use some improvement if there is a hermes-transform expert around.
Test plan
Before
node ./scripts/www/rewriteImports.js rewrites flow files correctly (using sketchy code)
After
node ./scripts/www/rewriteImports.js rewrites flow files correctly (using slightly better code)