Skip to content

Update library to use typescript #1

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

Merged
merged 1 commit into from
Mar 2, 2019
Merged

Update library to use typescript #1

merged 1 commit into from
Mar 2, 2019

Conversation

nggonzalez
Copy link
Owner

@nggonzalez nggonzalez commented Mar 2, 2019

Description

Modernize the library by adding Typescript and prettier, updating function names, creating smaller files, and updating dependencies.

Changes

  • Remove babel
  • Update ava dependency
  • Add typescript and tsconfig
  • Add tslint
  • Add husky and lint-staged
  • Export types

): string | never {
if (
Iterable.isIterable(data) ||
data instanceof Record ||
Copy link

Choose a reason for hiding this comment

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

There is a new function now that replaces Iterable.isIterable to check if something is Immutable: https://immutable-js.github.io/immutable-js/docs/#/isImmutable -- this will also cover Record.

... unless you're trying to support both Immutable 3 and 4? If so, then personally I'd suggest either something like:

const immutableCheckFn = typeof Immutable.isImmutable === 'function' ? Immutable.isImmutable : Iterable.isIterable;
if (immutableCheckFn(data)) { //...

or duck typing:

if (typeof data.toJS === 'function') { //...

Copy link
Owner Author

Choose a reason for hiding this comment

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

@idolize This doesn't update immutablet yet

Copy link

Choose a reason for hiding this comment

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

Ah, in that case the data instanceof Record check should never trigger since Iterable.isIterable returns true for Records as well.

decodedData = (RecordType as any).migrate(decodedData);
}

return new RecordType(decodedData);
Copy link

Choose a reason for hiding this comment

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

Is this guaranteed to be the right way to deserialize (constructor)? It will work with basic key/value pairs that don't require any transform from the server, but perhaps it would be nice to allow a custom decoder as an option, then fall back to constructor. Just a thought - not strictly necessary.

Copy link
Owner Author

@nggonzalez nggonzalez Mar 2, 2019

Choose a reason for hiding this comment

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

All of the existing tests pass, this is more of a build/format change. There is already a .migrate method that you can use to decode further (as seen above). Given that this is for next and that the encoding/decoding happens by the same library with no serialization plugins, the decoding should work

I can open an issue if you think it is important

Copy link

Choose a reason for hiding this comment

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

Oh nice, I missed that migrate

| OrderedMap<any, any>
| never {
const { __iterable: iterableType, data } = iterInfo;
switch (iterableType) {
Copy link

Choose a reason for hiding this comment

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

Nit: a map like { List: listDecoderFn, Set: setDeccoderFn, ... } would be slightly more performant than a switch.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good suggestion

@nggonzalez nggonzalez merged commit 8c7c523 into master Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants