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
[No QA] [TS migration] Migrate 'Config' files to TypeScript #37718
[No QA] [TS migration] Migrate 'Config' files to TypeScript #37718
Changes from 54 commits
7b98ae0
630b8ae
1dbe373
c6a484f
866211c
847b4b7
b29a9b3
7ad015d
ffc7b3d
21f6328
5a6b1ee
59522b7
1819da7
05b5c43
8bcb078
56e5b17
b80efcc
7649b02
a0ba014
512cf42
5ef518a
677243b
77a9b0d
0141669
3c29359
ca48f2c
dafc6bb
21721cf
ec234b6
a3e32ed
73406a6
d11e749
e09aadf
acedc7f
1a31c28
b2fb18f
7dc8d4d
0984f64
b05ea1a
433c321
ffdcc2b
55eb2d3
bbe50b6
cc93841
e059dd1
0781659
b44cd0f
170b3fe
6ec96dd
5bb52b4
d38ea29
bfa7795
39fe477
80796d3
0756e49
4c9144b
ed108c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
💚
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.
Mentioning this again since you 👍 'd even though I said it's NAB. Are we updating all the
config
s toconfiguration
s or not? Just curiousThere 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 going to change it, thanks
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.
Is this is the only place we use
??=
? I think this could be a bit more readable without that, but that's probably because I just learned this is valid syntax, so NABThere 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 you suggest we change it?
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.
Up to you, I don't feel very strongly, specially given the file it's in
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.
Is this comment above the line below it?
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 think it's above, not 100% sure what the author had in mind 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.
🙈 I phrased that very poorly, but I think you got it. How does this line expose react-native-config to desktop-main? Isn't that done by
plugins: [definePlugin],
? this isn't a blockerThere 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.
By "author" I meant the person that added the comment 😄
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.
Do we need the
?
inplugin?.constructor
?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.
Yes,
plugin
might be null or undefinedThere 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.
Interesting. I quickly searched for the type of
rendererConfig.plugins
to try to figure this out and didn't find it, and I was assuming that if we can iterate overrendererConfig.plugins
, then the items in it wouldn't be null or undefinedThere 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.