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

Convert project to Typescript and add es2015 build target #672

Merged
merged 7 commits into from Oct 16, 2019

Conversation

@csvn
Copy link
Contributor

csvn commented Oct 11, 2019

This PR tries to fix #644, which happens when native classes are used via RxJS from the es2015 package.json property. This library then crashes due to the transformed classes are trying to call class constructors instead of using new. RxJS has main, module and es2015 fields in their package.json. This PR adds es2015 to this library as well, so native classes are used at the same time if the same package.json fields are used.

Updated implementation notes

  • All source/test files have been migrated to Typescript
  • Typescript version has been updated to ^3.6.4 from ^2.1.5
  • Uses strict and importHelpers in tsconfig.json
  • Linting is still done via eslint, but @typescript-eslint is used to add Typescript support as well as a few recommended Typescript rules
  • Files are transpiled via Typescript instead of babel/gulp
  • Bundles are still built with Webpack, but now uses ts-loader instead of babel-loader

Original implementation notes (babel)

Babel is not well suited to transform to a specific language level, so transforming to ~es2015 is kind of hacky in this implementation. The best solution would be to instead compile this library as RxJS does; via Typescript. I did not want to do that in this PR without checking if there was any interest in this. There are several benefits to using Typescript instead of babel:

  • RxJS and redux-observable are used together, using similar tooling might be beneficial
  • It would be easier to generate es2015 the same way as RxJS
  • Less extraneous runtime helpers (e.g. _createClass, _inherits, _classCallCheck) that are repeated throughout the compiled redux-observable code (RxJS uses tslib, so all helper functions are imported)
  • If files are changed to Typescript, index.d.ts would no longer need to be maintained

The lib/es2015 folder is ~4KB smaller (11KB -> 7KB) on my machine, and much of this can be attributed to most runtime helpers no longer being necessary. If this project was compiled with TS and importHelpers, the cjs and esm builds would get smaller too.

If converting to .ts, or just compiling .js with Typescript would be agreeable, let me know and I'll update the PR.

Updated 1: New implementation in Typescript instead of babel

@csvn csvn changed the base branch from es2015 to master Oct 11, 2019
@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Oct 12, 2019

This is AWESOME!! and I'm definitely down for porting to TypeScript--it's long overdue tbh Did you volunteer? If you don't have cycles, I should be able to do it soon. If it's easier for a first pass, feel free to skip strict options like no implicit any, non-nullables, etc and I can come in and fix it if there are cases where we're doing things that might require heavy refactoring to make type safe/not awkward.

Thank you!! <3

Edit: re-read and it does indeed sound like you volunteered. Thank you!

@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 15, 2019

@jayphelps We use Typescript with all our projects, so I wouldn't mind taking a stab at this!

Copy link
Contributor Author

csvn left a comment

@jayphelps I've made a first iteration that seems to be working as it should. npm test passes without linting errors, build errors, or test errors. In general I tried to keep this PR "small" in order to avoid bugs/breaking-changes:

  • Minimal changes to source code, mostly by using Typescript features (private, class properties, etc)
  • I tried to avoid changing the public API types from the old index.d.ts, and mostly moved the existing types to appropriate source code
  • There are a few TODOs where the typing seemed a bit off. I can remove these, change the code, or just leave the comments depending on preference
  • Due to the Typescript migration, linting now affects typings.ts when it previously did not, so I fixed the lint errors in this file
  • Some lint rules have been turned off since some are already handled by Typescript

I'll try to respond in a timely manner to any questions 🙂

src/StateObservable.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
src/createEpicMiddleware.ts Show resolved Hide resolved
src/createEpicMiddleware.ts Outdated Show resolved Hide resolved
configs/tsconfig.defaults.json Show resolved Hide resolved
@csvn csvn changed the title Add es2015 build target to match RxJS Convert project to Typescript and add es2015 build target Oct 16, 2019
@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 16, 2019

There was a CI error due to a some changes made for @typescript-eslint at version 2.4.0. It should be fixed in next version (typescript-eslint/typescript-eslint#1088). The PARSER_NO_WATCH=true environment variable should be able to be removed then.

Copy link
Member

jayphelps left a comment

Overall this is simply wonderful. I only have a few questions inline that once we decide on, this will LGTM

test/typings.ts Outdated Show resolved Hide resolved
src/StateObservable.ts Outdated Show resolved Hide resolved
src/combineEpics.ts Outdated Show resolved Hide resolved
src/combineEpics.ts Outdated Show resolved Hide resolved
src/createEpicMiddleware.ts Show resolved Hide resolved
src/createEpicMiddleware.ts Outdated Show resolved Hide resolved
src/createEpicMiddleware.ts Outdated Show resolved Hide resolved
src/operators.ts Show resolved Hide resolved
@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Oct 16, 2019

The failing test const rootEpic = combineEpics(() => 'anonymous', epic2); is invalid, the epic shouldn't return a string. (this code was already there, not something you added)

@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 16, 2019

Yeah, forgot to run tests before I made the commit. I'll update the PR shortly.

@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 16, 2019

Again I forgot to fix run tests before comitting. I'll try to fix it tonight. 😅

@csvn csvn requested a review from jayphelps Oct 16, 2019
@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 16, 2019

Tests are passing again, and I think I've addressed your comments. I decided not to change all the tests that breaks the typing. Some are intentionally verifying that error is thrown if epic does not return a stream. Some tests are just simpler of they do not send exact correct types to an epic. I can of course adjust this if you think that's appropriate.

@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Oct 16, 2019

All LGTM. If anyone has objections or changes, lmk, but I'm gonna merge for now.

Won't be releasing this immediately, as I'd like to do this under 2.0.0 major version bump and there are some other things that are almost ready for that.

If anyone is interested in alpha testing a build of this, I can publish as a special tag to npm, just lmk. Otherwise I'll chime in when I've done that anyway, probably in a couple days.

@jayphelps jayphelps merged commit ba4699e into redux-observable:master Oct 16, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Oct 16, 2019

Thanks again @csvn 🎉 !

@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Oct 17, 2019

Glad I could help @jayphelps! 😀

@csvn csvn deleted the csvn:es2015 branch Oct 17, 2019
@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Nov 14, 2019

Hey @jayphelps, sorry to bother you. I'd be interested to try out an alpha of the 2.0.0 version if possible, unless the latest release is close. No pressure though 🙂

@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Nov 14, 2019

@csvn appreciate the apology, but it's perfectly all right! Indeed wanted you to let me know if/when a pre-release would be helpful. I'll try and get one in the next day or two and will update here.

@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Nov 15, 2019

I've cut a pre-release 2.0.0-alpha.0 or the next tag (which will be updated to whatever the latest pre-release is.

@csvn

This comment has been minimized.

Copy link
Contributor Author

csvn commented Nov 15, 2019

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.