Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix #445 return type for Header#get #473

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Fix #445 return type for Header#get #473

merged 2 commits into from
Aug 20, 2021

Conversation

armanbilge
Copy link
Member

No description provided.

@japgolly
Copy link
Contributor

  1. Are you absolutely sure it will never return undefined in any circumstance/browser?
  2. Breaks compat. Will have to target 2.x

@armanbilge
Copy link
Member Author

Good questions 🤔

  1. Not according to the spec. I have no idea how to answer that question in general 🙃
  2. I'm still confused about what exactly breaks compat. I assume this is breaking source compat but not binary compat?

@japgolly
Copy link
Contributor

  1. Gotta just google it I'm afraid. I usually start with mdn, then if needed read through the linked specs.
  2. Hmmm interesting, actually my understanding of binary compat when it comes to SJS facades is full of unverified assumptions so I'm not much of an authority there. But source compat definitely breaks. Imagine .get(???).isDefined

@armanbilge
Copy link
Member Author

  1. Yup, I checked in MDN and it didn't say anything about undefined, just possibly returns null.
  2. I got an excellent primer on binary compat for facades here: Enable MiMa #461 (comment)

Regarding source compat: we should have clarified what freezing backwards compat looks like for the next 1.x. I assumed we'd freeze bincompat (semantic versioning) but not source compat. If we also freeze source compat, then many PRs are no longer applicable to 1.x. I'm fine either way, let's just be really clear on this :)

@japgolly
Copy link
Contributor

Yeah actually of course, what am I thinking, you're right. Bin-compat's the goal.

@sjrd
Copy link
Member

sjrd commented Aug 12, 2021

I can confirm that this PR does not break binary compatibility. (In a Scala class, it would, but not in a JS facade.)

@japgolly japgolly marked this pull request as draft August 12, 2021 23:55
@japgolly japgolly added this to the v1.2.0 milestone Aug 13, 2021
@japgolly japgolly self-requested a review August 20, 2021 02:54
@japgolly japgolly marked this pull request as ready for review August 20, 2021 02:54
@japgolly japgolly merged commit c9f60ce into series/1.x Aug 20, 2021
@japgolly japgolly deleted the issue/445 branch August 20, 2021 02:57
@japgolly
Copy link
Contributor

Thanks @armanbilge 👍

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

Successfully merging this pull request may close these issues.

3 participants