-
-
Notifications
You must be signed in to change notification settings - Fork 252
Migrate to rescript webapi #1063
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Deploying rescript-lang-org with
|
Latest commit: |
9e12478
|
Status: | ✅ Deploy successful! |
Preview URL: | https://559ff893.rescript-lang.pages.dev |
Branch Preview URL: | https://experimental-webapi.rescript-lang.pages.dev |
src/common/MetaTagsApi.res
Outdated
let nodeList = document->WebAPI.Document.querySelectorAll("meta") | ||
|
||
let name = switch (name, property, itemprop) { | ||
| (Some(name), _, _) => Some(name) | ||
| (_, Some(property), _) => Some(property) | ||
| (_, _, Some(itemprop)) => Some(itemprop) | ||
| _ => None | ||
} | ||
let nodesArray = [] | ||
|
||
let content = meta->Element.getAttribute("content")->Nullable.toOption | ||
for i in 0 to nodeList.length { | ||
let node = WebAPI.NodeList.item(nodeList, i) | ||
nodesArray->Array.push(node) | ||
} | ||
|
||
switch (name, content) { | ||
| (Some(name), Some(content)) => tags->Dict.set(name, content) | ||
| _ => () | ||
} | ||
let metaTags = nodesArray->Array.reduce(Dict.fromArray([]), (tags, meta) => { | ||
let name = meta->Obj.magic->WebAPI.Element.getAttribute("name") | ||
|
||
tags | ||
}) | ||
let content = meta->Obj.magic->WebAPI.Element.getAttribute("content") | ||
tags->Dict.set(name, content) | ||
tags | ||
}) |
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'm not sure the best way to do this. In this code I need to access the attribute for each element, however, WebAPI.Document.querySelectorAll
returns WebAPI.DOMAPI.nodelist
and not an array<WebAPI.DOMAPI.element>
. There are several Obj.magic
Some suggestion?
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.
You could try the https://developer.mozilla.org/en-US/docs/Web/API/NodeList/values
Or https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach
In JS, you can spread a NodeList into an array like [...document.querySelectorAll(".blah")
.
But you do have with node
interface. You could do instanceof Element
interopt check, but you will need to cast at some point.
@@ -1510,26 +1508,34 @@ let make = (~versions: array<string>) => { | |||
let subPanelRef = React.useRef(Nullable.null) | |||
|
|||
let onResize = () => { | |||
let newLayout = Webapi.Window.innerWidth < breakingPoint ? Column : Row | |||
let newLayout = window.innerWidth < breakingPoint ? Column : Row | |||
setLayout(_ => newLayout) | |||
switch panelRef.current->Nullable.toOption { | |||
| Some(element) => |
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.
Here, element
of type WebAPI.DOMAPI.element
dont have the style
property.
I could modify the height access the height
property -> element.style.height = "100px"
, instead of use WebAPI.Element.setAttribute
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.
instanceof HTMLElement
and cast to htmlElement
maybe?
This is not ideal and brought forward rescript-lang/experimental-rescript-webapi#5 and rescript-lang/experimental-rescript-webapi#13
@@ -745,7 +738,7 @@ module WarningFlagsWidget = { | |||
let suggestionBox = | |||
Option.map(suggestions, elements => | |||
<div | |||
ref={ReactDOM.Ref.domRef(listboxRef)} | |||
ref={ReactDOM.Ref.domRef((Obj.magic(listboxRef): React.ref<Nullable.t<Dom.element>>))} |
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.
ReactDOM.Ref.domRef
except React.ref<Nullable.t<Dom.element>>
, but this have type React.ref<Nullable.t<WebAPI.DOMAPI.htmlElement>>
switch element->Element.contentWindow { | ||
| Some(win) => win->Element.postMessageAny({code, imports}, ~targetOrigin="*") | ||
| None => Console.error("contentWindow not found") | ||
let element: WebAPI.DOMAPI.htmliFrameElement = element->Obj.magic |
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.
Cast to WebAPI.DOMAPI.htmliFrameElement
src/Packages.res
Outdated
"name": String(name), | ||
"keywords": Array(keywords), | ||
"version": String(version), | ||
"links": Object(dict{"npm": String(npmHref)} as links), |
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.
Testing new dict pattern matching. @zth I can match optional fields (ex: dict{"npm": String(npmHref), "repository": ?Some(String(repo))}
)?
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.
Correct! It's an optional field underlying so you need to specify if you want to match on the optionality.
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.
Nice!!
src/common/MetaTagsApi.res
Outdated
meta | ||
->WebAPI.Element.getAttribute("name") | ||
->Nullable.make | ||
->Nullable.toOption |
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 the Nullable.toOption
is unnecessary - we can match on Value(name)
etc. below instead.
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.
Also very weird that getAttribute
does not return nullable<string>
or even null<string>
as the docs clearly state that that could be the case.
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.
Fixed 9e12478
No description provided.