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

Bindings for IntersectionObserver #27

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

illusionalsagacity
Copy link
Contributor

@illusionalsagacity illusionalsagacity commented Sep 17, 2021

ported over from reasonml-community/bs-webapi-incubator#197

Have a few questions on this one:

  • should we continue doing the functions for properties? Personally I kinda like the potential for sharing the Dom phantom types for inter-library compatibility
  • Using makeX named parameter constructor functions, vs the new @obj on records for next rescript release.

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.

This seems fine, I'm not familiar with the intersection observer API so it's hard to comment on the correctness of the bindings directly.

should we continue doing the functions for properties? Personally I kinda like the potential for sharing the Dom phantom types for inter-library compatibility

yes. I speculated about this in #2, and discussed it on the reasonml discord, for the moment consensus is that the webapi should maintain compatibility with other code that uses Dom types.

Using makeX named parameter constructor functions, vs the new @obj on records for next rescript release

I haven't heard about this, where was it announced? I just found the compiler changes, that looks cool.

Unfortunately I don't think it's appropriate for this library to use new compiler features that quickly - not everyone upgrades immediately. If we haven't released 1.0 by ~6 months after rescript 10, then I'll consider it as a breaking change.

src/Webapi/Webapi__IntersectionObserver.res Outdated Show resolved Hide resolved
@illusionalsagacity
Copy link
Contributor Author

This seems fine, I'm not familiar with the intersection observer API so it's hard to comment on the correctness of the bindings directly.

If I remember correctly I was testing this via the test file in an html doc in a browser. Might be something to address elsewhere as well?

yes. I speculated about this in #2, and discussed it on the reasonml discord, for the moment consensus is that the webapi should maintain compatibility with other code that uses Dom types.

Yeah I think the only downside here is bs-webapi-incubator wasn't using the Dom phantom types comprehensively, but maybe that would be worth doing prior to 1.0 for this library?

I haven't heard about this, where was it announced? I just found the compiler changes, that looks cool.

Unfortunately I don't think it's appropriate for this library to use new compiler features that quickly - not everyone upgrades immediately. If we haven't released 1.0 by ~6 months after rescript 10, then I'll consider it as a breaking change.

Yeah it was a little buried but it was in here https://forum.rescript-lang.org/t/rfc-more-general-type-checking-for-structural-typings/1485/51

@TheSpyder
Copy link
Owner

This seems fine, I'm not familiar with the intersection observer API so it's hard to comment on the correctness of the bindings directly.

If I remember correctly I was testing this via the test file in an html doc in a browser. Might be something to address elsewhere as well?

Better testing would indeed be awesome. Even if it's just cypress or something using headless chrome, at least it will be some sort of browser verification. It's not on the roadmap at this stage.

yes. I speculated about this in #2, and discussed it on the reasonml discord, for the moment consensus is that the webapi should maintain compatibility with other code that uses Dom types.

Yeah I think the only downside here is bs-webapi-incubator wasn't using the Dom phantom types comprehensively, but maybe that would be worth doing prior to 1.0 for this library?

bs-webapi definitely did use the BuckleScript (now ReScript) Dom types, where they were available. We haven't changed that (and I don't think we're really likely to).

I haven't heard about this, where was it announced? I just found the compiler changes, that looks cool.
Unfortunately I don't think it's appropriate for this library to use new compiler features that quickly - not everyone upgrades immediately. If we haven't released 1.0 by ~6 months after rescript 10, then I'll consider it as a breaking change.

Yeah it was a little buried but it was in here https://forum.rescript-lang.org/t/rfc-more-general-type-checking-for-structural-typings/1485/51

thanks for the link!

@illusionalsagacity illusionalsagacity marked this pull request as ready for review September 22, 2021 00:36
@TheSpyder TheSpyder merged commit 0e0733e into TheSpyder:main Sep 22, 2021
@TheSpyder
Copy link
Owner

Published 0.1.2

@illusionalsagacity illusionalsagacity deleted the intersection-observer branch September 22, 2021 03:02
@TheSpyder
Copy link
Owner

I've just remembered a good reason to keep the files separate - tree shaking. Tree shaking can remove top-level functions but not nested module definitions (because there's no good system to mark them as pure).

Having said that, with intersection observer (and resize observer) it's not really possible to use the parent module without the sub module - so it shouldn't make much difference.

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.

2 participants