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

WIP: Spec fix #198

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

WIP: Spec fix #198

wants to merge 4 commits into from

Conversation

dead-claudia
Copy link

@dead-claudia dead-claudia commented Aug 16, 2018

Fixes: #197
Fixes: #196
Fixes: #186
Fixes: #184
Fixes: #176
Fixes: #165
Fixes: #91

This doesn't yet include updated tests, but I did update the spec.

I purposefully tried to leave out spec changes/fixes that might be controversial, like changing/removing the 3-arg subscribe variant or not swallowing errors. But here's a list of what I did fix (I think this is all):

  • I fixed a slew of erroneous non-throwing assertions.
  • I fixed a few typos, formatting errors, and other related mistakes and inconsistencies.
  • I fixed Observable.from to correctly use @@iterator for the iterable case.
  • I edited the "HostReportErrors" stuff to just use a common shorthand instead, so it's a bit more readable and intelligible, and to fix a few inconsistencies. Implementations are likely to just use a macro or do what polyfill writers do and desugar them into try/catch blocks.
  • I merged the next/error/complete operations to just use variants of the same core algorithm. I also used it to replace the erroneous object method call within Observable.prototype.subscribe. This simplified the spec tremendously without loss of clarity.
  • I added quite a few spec assertions over internal slots and their types, for clarity.
  • I fixed the type checks that went "if O does not have the [[Foo]] internal slot" to read "if O does not have all of the internal slots of Bar objects" instead, for consistency and clarity.
  • I fixed all the subscription observer callbacks for Observable.from and Observable.of to 1. type-check their input, and 2. work on their arguments generically instead of assuming they're subscription observers.
  • I added @@toStringTag support for the various types (observable, subscription, and subscription observer).
  • I elected to defer the unsubscribe method check instead of accessing it twice. I still maintained the object check so engines can avoid handling nonsensical cases.
  • I changed all the internal slot accesses to match the mainline spec - it's way more concise.
  • I fixed a lot of consistency issues within the README and clarified a few things with (usually) added annotations. I also changed it to use TypeScript highlighting, so it's less likely to be odd and unusual in editors.

I'm aware it reads a bit like shotgun surgery, but when I was trying to write an optimized implementation of this (with JIT-compiled pipelines), trying to figure out the behavior I needed to maintain required so many explicit spec deviations I decided my time was better spent fixing the spec so I could do the experiment. And yes, a lot of things needed fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment