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

fix(Notification): replace const enum #4556

Merged
merged 2 commits into from Apr 23, 2019

Conversation

@cartant
Copy link
Collaborator

commented Feb 11, 2019

Description:

This PR replaces the NotiticationKind const enum that was introduced in 6.4.0 with a string-literal union type. The latter is type-safe and compatible with single-module transpilation. Const enums are not, as the transpiler cannot know what should be used as a replacement for a reference to a const enum without using type information.

The NotiticationKind const enum is a problem for any projects using the --isolatedModules flag or for projects using the Babel TypeScript transform - which means any TypeScript projects generated with create-react-app.

Related issue (if exists): #4538

@cartant

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 11, 2019

BTW, I attempted to add a transpile script to reproduce the error - and guard against it in the future - but I was unable to get it to work, as TypeScript seems to only perform the check that effects that error for packages consumed from node_modules. Consuming them from dist effects a truckload of other errors about re-exports also not being compatible with --isolatedModules.

@coveralls

This comment has been minimized.

Copy link

commented Feb 11, 2019

Pull Request Test Coverage Report for Build 8266

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.3%) to 96.687%

Files with Coverage Reduction New Missed Lines %
src/internal/observable/ConnectableObservable.ts 1 55.46%
src/internal/operators/partition.ts 1 75.0%
src/internal/Observable.ts 1 91.67%
src/internal/util/not.ts 4 20.0%
Totals Coverage Status
Change from base Build 8252: -0.3%
Covered Lines: 5253
Relevant Lines: 5433

💛 - Coveralls
@mrmckeb

This comment has been minimized.

Copy link

commented Mar 21, 2019

This is great, and will allow us to move to v6.4. Are there any concerns about merging this in?

@cartant cartant requested a review from benlesh Mar 21, 2019
@benlesh

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Unfortunately, this is a breaking change. Anyone using the NotificationKind enum like NotificationKind.NEXT will be broken with this change.

@benlesh

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I think if we just leave the enum where it is, and convert everything else to just use 'N'|'E'|'C' we should be able to resolve the bug.

We can then @deprecate NotificationKind.

@cartant

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

@benlesh Unfortunately, this has to be a breaking change. It's the export that is the problem - regardless of whether or not NotificationKind is used:

node_modules/rxjs/internal/Notification.d.ts:3:27 - error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided.
3 export declare const enum NotificationKind {

The introduction of the const enum was a breaking change for many people - myself included - so I see no reason the fix cannot also be a breaking change.

@kwonoj

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Maybe just const enum to enum?

The enum is kept, but it is no longer a const enum. It cannot be exported as a const enum without effecting an error if isolated modules are used.
@cartant cartant force-pushed the cartant:issue-4538 branch from 5d3a78c to ca0afc0 Mar 26, 2019
@cartant

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

@benlesh I've updated the PR to use a literal union in the Notification signature - with NotificationKind being kept as a non-const enum. A const enum cannot be exported without effecting an error in situations in which isolated modules are used.

I've added a deprecation - with a message - to the enum. In the next major version, the enum could be ditched and NotificationKind could be declared as a type alias for the literal union.

@mrmckeb

This comment has been minimized.

Copy link

commented Apr 12, 2019

It looks like this is actually passing, the Travis error seems to be unrelated. Any chance we can get this in now?

@benlesh benlesh merged commit e460eec into ReactiveX:master Apr 23, 2019
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.3%) to 96.687%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
BioPhoton added a commit to BioPhoton/rxjs that referenced this pull request May 15, 2019
* fix(Notification): replace const enum

Closes ReactiveX#4538

* chore: use literal union and keep enum

The enum is kept, but it is no longer a const enum. It cannot be exported as a const enum without effecting an error if isolated modules are used.
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.