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

Update ts: alternative to #5 #6

Merged
merged 2 commits into from
Jun 3, 2019
Merged

Update ts: alternative to #5 #6

merged 2 commits into from
Jun 3, 2019

Conversation

JirkaVebr
Copy link
Contributor

This is a largely unfortunate duplicate of #5, although it, I believe, addresses the issues at hand slightly better.

Rationale comparing these changes to #5:

  1. TS can be somewhat unpredictable at times, and so fixing its version to ^3.5.1 shoul be more stable, and thus preferable to ^3.5.x. We can always release new versions if need be.
  2. The casts in Component.ts are strictly more specific than those in Upgrade TypeScript to 3.5 and fix compilation errors #5, and thus are slightly less of an unfortunate workaround.
  3. The early return in InView.ts is more elegant than a large branch.
  4. Albeit at the expense of an as any cast, the changes in inject.ts still rely on setting properties as opposed to attributes, which is the correct approach.

@JirkaVebr JirkaVebr requested review from ViliamKopecky and enzy June 3, 2019 11:53
Copy link
Contributor

@enzy enzy left a comment

Choose a reason for hiding this comment

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

What about the custom HTMLElementEventMap interface. Why is it still used?

@JirkaVebr
Copy link
Contributor Author

@enzy Well, it's a rather unfortunate workaround but it still has its place. The problem is that the official TS HTMLElementEventMap only includes events whose W3C specifications are candidate recommendations or further. However, that excludes focusin and focusout as these are still just editor's draft. Despite that, they have practically universal browser support, and thus our library adds them so that people can listen to them. Essentially, our extension will eventually get removed but I'm afraid the time has not come yet. 😐

@enzy
Copy link
Contributor

enzy commented Jun 3, 2019

Oh, it's this part <E extends keyof HTMLElementEventMap> I haven't noticed. Okay 👍

@JirkaVebr JirkaVebr merged commit a431f76 into master Jun 3, 2019
@enzy
Copy link
Contributor

enzy commented Jun 3, 2019

setting properties as opposed to attributes, which is the correct approach

→ because properties are typed, attributes are just strings 👍

@enzy enzy deleted the pr/update-ts branch June 3, 2019 12:19
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