Conversation
|
Migration should be easy if the rescript feature for tagged variants is accepted. Example with the current PR let textData = switch element.childNodes
->DOMApi.NodeList.item(0)
->Null.map(node => node->DOMApi.NodeTypeHelper.findNodeType) {
| Null.Value(Text(text)) => text.data
| _ => "not a text node"
}Example with the rescript feature for tagged variants. let textData = switch element.childNodes->DOMApi.NodeList.item(0) {
| Null.Value(Text(text)) => text.data
| _ => "not a text node"
} |
src/types/DOMAPI.res
Outdated
|
|
||
| module Document = { | ||
| include Node.Impl({ | ||
| type t = element |
There was a problem hiding this comment.
Shouldn't that be
| type t = element | |
| type t = document |
?
src/types/DOMAPI.res
Outdated
| | Document(document) | ||
| | Unknown | ||
|
|
||
| let findNodeType = (node: node) => |
There was a problem hiding this comment.
This could be called classify like similar functions in the stdlib. And the module could be NodeType. So that you would have NodeType.classify returning a NodeType.t.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c58b72837
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type t = element | ||
| }) | ||
|
|
||
| include DOMAPI.Element.Impl({ |
There was a problem hiding this comment.
NativeElement now includes the new DOM-style methods on every ref, but this package still advertises react-native >=0.81.0 support. React Native only started returning DOM-like nodes from refs in 0.82, so an app that is still on 0.81 can compile against these bindings and then hit undefined is not a function at runtime as soon as it calls one of the newly exposed methods.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It will be done before release
src/types/DOMAPI.res
Outdated
| @send | ||
| external compareDocumentPosition: (T.t, node) => int = "compareDocumentPosition" | ||
| @send external contains: (T.t, node) => bool = "contains" | ||
| @send external getRootNode: T.t => T.t = "getRootNode" |
There was a problem hiding this comment.
getRootNode() is typed as returning T.t, but for mounted element and text nodes the root is the document node, not another element/text node. The current signature is only correct for the detached-node case mentioned in the React Native docs, so common mounted calls will be typed as element/text and let callers use subtype-specific fields on a document object at runtime.
Useful? React with 👍 / 👎.
src/types/DOMAPI.res
Outdated
| type t = document | ||
| }) | ||
|
|
||
| @send external getElementById: (document, string) => element = "getElementById" |
There was a problem hiding this comment.
getElementById() can legitimately miss when no native component has the requested id/nativeID, but this binding models it as always returning element. That makes the absence case impossible to express in ReScript and turns a normal lookup miss into a null dereference when consumers immediately access fields or methods on the result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch, I'll also open a PR on webapi
src/types/DOMAPI.res
Outdated
| type t = text | ||
| }) | ||
|
|
||
| @send external substringData: (text, ~offset: int, ~count: int) => unit = "substringData" |
There was a problem hiding this comment.
substringData() returns the extracted text, but the external is declared as unit. As written, the new Text-node binding silently discards React Native's return value, so callers cannot actually read any substring from a text node through this API.
Useful? React with 👍 / 👎.
|
@Freddy03h I think Codex has some good points here, can you check them? |
|
I have a concern: // NativeElement.res
type element = DOMAPI.element
// ScrollViewElement.res
type elementIf I change ScrollView’s element to |
|
That is a good point. I think we need to keep the element types abstract to avoid this issue, and offer "casts" using scrollViewElement->ScrollViewElement.asNativeElement->NativeElement.asDOMElementand/or directly scrollViewElement->ScrollViewElement.asDOMElement |
Blog Post : Starting from React Native 0.82, native components will provide DOM-like nodes via refs.
Documentation : Nodes from refs
I tried to follow the approach used in experimental-rescript-webapi but I hope this rescript feature for tagged variants can be addressed soon, as it would provide a better DX.
In the meantime, I added a
NodeTypeHelpermodule to help with type conversions.I know it doesn’t follow the zero-runtime binding rule, but while experimenting with
experimental-rescript-webapi, I found this kind of helper quite useful, and it would otherwise have to be reimplemented in every project.