Tweak syntax in rollup build script #9852
Conversation
scripts/rollup/build.js
Outdated
| headerSanityCheck, | ||
| }) { | ||
| /** | ||
| * @param configs { |
There was a problem hiding this comment.
I'm okay with this but we generally are bad at maintaining JSDocs. They always eventually get out of sync with the code.
Maybe we could use Flow here instead?
There was a problem hiding this comment.
I'm also curious about the motivation — is it because the code is hard to read? Or it doesn't run in some Node version? It it's about the declaration feeling crowded, maybe we could keep the destructuring but move it into a first statement. I'm just hesitant to add comments that aren't checked by a tool, and pretty much repeat the code.
There was a problem hiding this comment.
Or it doesn't run in some Node version?
This was the reason.
I'm just hesitant to add comments that aren't checked by a tool, and pretty much repeat the code.
Agreed that comments are kind of no-value-add here. They're a bit of extra maintenance burden but otherwise you might as well just scan below.
There was a problem hiding this comment.
Sounds good - will remove the comments.
@bvaughn and I already discussed this. **test plan:** `yarn build`
**what is the change?:** removing some comments **why make this change?:** The code is basically self explanatory and these comments could get out of sync. **test plan:** Visual inspection, `yarn build` and `yarn test` **issue:** facebook#9398
|
Tests are passing for me locally - I rebased, hopefully they will pass in CI. 🤞 |
|
Looks like CI's happy now too |
@bvaughn and I already discussed this.
test plan:
yarn build