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
RFC: React Native WebAPIs #2504
RFC: React Native WebAPIs #2504
Changes from 16 commits
226451d
d9d634c
0f4775e
fed17d7
b706985
98b0ea2
e23792d
2a04215
9bf372b
cb9f806
8280738
5455b97
4508f3a
8409c71
0567637
1c32d86
714568b
7baefbc
18a3e1d
5684aba
cca0e1e
df23ecb
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.
I've been thinking about this too but the direction I was considering was a bit different. If we use the dependencies listed in
package.json
and the current autolinking approach, we might end up including JS and native code in the binary that's not actually used. The approach I was considering instead was doing 2 things:Optionally, we could also make it so the resolved module is configurable by the user, so we can actually swap implementations whenever necessary.
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.
Hi @rubennorte! Thanks for your feedback. I had some thoughts around these approaches as well (that I probably should put somewhere in the document),
navigator
doesn't refer to something else? I think the same problem applies toRange
and other APIs? I'm not so well-versed in Babel or AST manipulation in general, so please forgive the dumb questions.I'm thinking that we want to align with the behaviour of native autolinking i.e., we should allow autopolyfilling to be disabled via the very same mechanisms. If we can tell people that it works the exact same way native autolinking does, that will reduce cognitive load. We should aim to fix native autolinking behaviour as necessary. I'm a bit wary of introducing yet another concept and making it hard to debug/wrap your head around.
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.
Yeah, I think there are cases where forcing this is reasonable (including when using OTA), so we definitely need a way to force certain native modules to always be included regardless of whether they're used or not. We probably shouldn't be doing that by default though.
In the future, if we shipped the browser APIs with RN, we could have an option in the RN config like:
I think that tool would be useful but only in the case we discussed before (using OTA or SSR). For the metadata, my approach was about native modules, not about Web APIs specifically (which has the added benefit of reducing bloat in a broader set of cases).
We can tell if the reference to
navigator
is global or if it's redefined/imported from another module. The only requirement from this would be that you use it directly (as innavigator.getBattery()
) instead of using indirection (e.g.:const {getBattery} = navigator; getBattery();
).I think it's fair to focus on the current capabilities in RN to implement this, but we need to make sure it wouldn't conflict with the direction that we want to take in the long term.
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.
@tido64 could you use a specific example here? I'm not sure I understand the shape of this config 100%.
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.
At the moment, there really isn't much else to it. We're building on top of the existing autolinking schema as defined in the CLI: https://github.com/react-native-community/cli/blob/main/docs/autolinking.md#what-do-i-need-to-have-in-my-package-to-make-it-work
Just to be clear, this
react-native.config.js
lives in the module package. Not in the app project.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.
cc @motiz88. Flagging in case you have any concerns about 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.
I think it's important for this to live in the
react-native
repo and not be another 3rd party project that people have to go looking around for to find modules. My position is that the DOM/Web APIs is the long-term future of the maintained RN JS APIs, and it's critical that they be hosted and maintained out of the main monorepo infra.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.
hey @necolas, it's great to know that we are very much aligned in exploring this direction 🤗 about this specific angle of where the code should live, while I agree that in the long-term all this xplat code should live together, I think that while this is still very much experimental it'd be better to keep it somewhere else, and then potentially migrate it in the react-native repo if/when we validate that it's viable.
I'm concerned that moving the code into rn now will create a lot of overhead in both maintenance and contribution speed, and during this first experimental phase we want to be able to move swiftly without worrying inadvertently breaking something else.
Let's chat again about this further down the line?
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.
Depends how long "experimental" goes on for. We're currently greatly slowed by a lot of past forking and out-of-repo initiatives, and have long and complicated projects to defork those things. That needs to be avoided with new work.
We also have existing Web APIs that we've implemented and they're not separate repos or projects either. They'll be highly dependent on new internals like an event loop model and native extension points, so being out of core will make it harder for Web APIs with those needs to be tested and inform development.
Curious to hear @rubennorte's thoughts
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.
@necolas I think colocation is more critical for APIs that interact with internals from RN (like all the DOM traversal & layout APIs that interact with Fabric), but it might not be as important for standalone APIs like Battery, etc. that could be easily implemented as a userland module. I agree with the general direction of eventually having this provided by the framework out of the box, but I don't think it's that important at this point (and it'll help with iteration speed).
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.
See my previous comment about autolinking and how it could completely solve the discovery problem :)
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.
Agree that having auto-magic setup would be a great DX, and an essential part of opening up RN to developers coming from Web in the future.
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've been implementing MutationObserver and IntersectionObserver. Some Web APIs like those will come up against differences in the execution model of React Native vs Web, and we're working on an RFC to address that as well.
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.
another note: to help us prioritize against our internal needs, we've been creating a tool to help us scrape a codebase to find usages of every WebAPI so that we can be data-driven #2621