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] Interface all the things... #1853

Closed
wants to merge 3 commits into from

Conversation

david-driscoll
Copy link
Member

Description:
Let me prefix this with I really wish we didn't have to do this, but looking at the referenced issues I don't see the root problem being solved by the TypeScript compiler anytime soon.

There are problems when using rxjs with tools like npm link that cause the compiler to fail to resolve types properly and create lots of compiler errors, as it sees two copies of the same class, and they are not compatible.

The goal of this change is to do the following:

  • Make the public Observable an interface.
  • Make other public classes interfaces as well (Subscription, Subscriber, etc).
  • Make sure Observable is still the same Observable to external consumers.

Related issue (if exists):
microsoft/TypeScript#6496
microsoft/TypeScript#7755
#1744

Issues

  • Exporting new types will require jumping through similar interface hoops that this PR will solve.

Work Inprogress Todos

  • Rename Obsevable -> ObservableImpl
  • Rename IObservable -> Observable
  • Create ObservableStatic interface
  • Fixup TSLint errors
  • Update tests to use new interfaces (so they compile!)
  • Repeat renames for other interfaces such as Subscription (anything that the consumer may want to expose publicly)

cc @Blesh @robwormald @kwonoj @staltz

cc @mhegazy @ahejlsberg - maybe this can be fixed easily in the compiler? 🙏

As it stands rxjs (and any other library for that matter) cannot export any classes without causing potential compiler issues down the line for others. Essentially we're being penalized because we're using TypeScript to develop the library.

Are there other simpler fixes? Can the compiler emit an identifier in the .d.ts files that so that it knows "hey this is the same interface as the other one I have over here"?

@@ -65,7 +65,7 @@
"build_test": "shx rm -rf ./dist/ && npm-run-all build_cjs clean_spec build_spec test_mocha",
"build_cover": "shx rm -rf ./dist/ && npm-run-all build_cjs build_spec cover",
"build_docs": "npm-run-all build_es6_for_docs build_global build_spec tests2png decision_tree_widget && esdoc -c esdoc.json",
"build_spec": "tsc --project ./spec --pretty",
"build_spec": "tsc --project ./spec",
Copy link
Contributor

Choose a reason for hiding this comment

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

@david-driscoll why did you remove --pretty flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was to get errors to be detected by vscode, this will be restored once the branch gets ready for land.

Copy link
Contributor

Choose a reason for hiding this comment

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

aha. thank you

@david-driscoll
Copy link
Member Author

Closing this for now, as there may be an alternative... opening an issue for that.

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants