Skip to content
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

Add module functor to make CustomEvent with typed detail #93

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

mununki
Copy link
Contributor

@mununki mununki commented Jan 28, 2022

This PR is related to reasonml-community/bs-webapi-incubator#204, reasonml-community/bs-webapi-incubator#203.

This PR adds a module functor to make a CustomEvent module with typed detail.

Example

module OnChangeDetail = {
  type t = {
    component: string,
    valueAsDate: Js.Date.t,
    value: string,
  }
}

let bar: OnChangeDetail.t = {
  component: "date-picker",
  valueAsDate: Js.Date.make(),
  value: "2022-01-28",
}

module NewOnChangeEvent = Webapi.Dom.CustomEvent.Make(OnChangeDetail)
let eventWithDetail = NewOnChangeEvent.make("event-with-detail")

Js.log(eventWithDetail->Webapi.Dom.CustomEvent.preventDefault) // still can use as before.
Js.log(eventWithDetail->NewOnChangeEvent.preventDefault) // can use with new CustomEvent
Js.log2("make", NewOnChangeEvent.makeWithOptions("foo", bar)) // can make with options..
Js.log((eventWithDetail->NewOnChangeEvent.detail).valueAsDate) // typed detail!

@TheSpyder
Copy link
Owner

Interesting. I hadn't considered doing it this way. I'll give it some thought, thanks!

tests/Webapi/Dom/Webapi__Dom__EventTarget__test.res Outdated Show resolved Hide resolved
src/Webapi/Dom/Webapi__Dom__CustomEvent.res Outdated Show resolved Hide resolved
@mununki mununki force-pushed the customevent-typed-detail branch from 0de7ecd to 74c8e73 Compare February 4, 2022 07:48
@mununki
Copy link
Contributor Author

mununki commented Feb 4, 2022

committed the suggestions and fix the test.

Copy link
Owner

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I wonder whether we should delete the non-functor version in our v2 API update 😂

Comment on lines 36 to 38
let t: Detail.t = {
test: "test",
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done without the let t? I'm looking at the generated JS and it would be nice to avoid that part.

Copy link
Contributor Author

@mununki mununki Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mentioned this generated js part

var t = {
  test: "test"
};

I'm afraid that the omission of this let t can't be made unless we change the test case to make instead of makeWithOption. or renaming the let t would be nicer such as let option maybe?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why does it need to be separate let? It's just a simple record, can't it be EventWithDetail.makeWithOptions("event-with-detail", { test: "test" }) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misunderstood your intention. I removed it and pushed it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was two of them - sorry, please make that change to both :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I missed that, thx. I fixed it and pushed it. I reworded commit messages also.

@mununki mununki force-pushed the customevent-typed-detail branch from cabe968 to f620b55 Compare February 7, 2022 05:37
@mununki
Copy link
Contributor Author

mununki commented Feb 8, 2022

@TheSpyder I don't know why exactly, but does my rebase to reword the commits cause the task-list-complete check running loop maybe?

@TheSpyder
Copy link
Owner

No, it's because resolving a comment thread doesn't automatically resolve all tasks within it:
#93 (comment)

I normally take care of that before merging if it's clear the task was done.

I hope to find time to finish reviewing and merge this in the next day or two.

@mununki
Copy link
Contributor Author

mununki commented Feb 8, 2022

@TheSpyder I got it. Thx.

@TheSpyder TheSpyder merged commit d02122d into TheSpyder:main Mar 9, 2022
TheSpyder added a commit that referenced this pull request Mar 9, 2022
@mununki mununki deleted the customevent-typed-detail branch March 9, 2022 01:15
@TheSpyder
Copy link
Owner

published as 0.6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants