Migration to Typescript

[wf9a5m75]

Thank you for creating this library. I migrated current master branch code to TypeScript.

Writing TypeScript code gives you to some benefits:

• Easy to integrate with TypeScript
• Write code with clean code syntax
• Prevent type error
• Other people can easy to understand ...

I like pure JS for long years, but I like also TypeScript recently.

[Rycochet]

A couple of comments here (as it's on my stack but really don't know when I'll get time - so thanks!!!) -

• The jest config can go inside the package.json - personal preference only.
• Need to add tslint
• Need to build both standard and es6 outputs
• Why are there still space indents???? It's 2018 and space for indent has no place in modern code!!! (it didn't even have a place in code written 20 years ago either! Queue arguments lol)

Most important thing is getting some decent code quality in via tslint - can reorganise the files better at a later point ;-)

[wf9a5m75]

I already included tslint from the first.

https://github.com/wf9a5m75/lz-string/blob/f342fe51bd5c007377dd8140daeed549d3838930/package.json

---

Need to build both standard and es6 outputs

Why your code is written in es3?

If you prefer standard and/or es6, please change the settings. https://github.com/wf9a5m75/lz-string/blob/f342fe51bd5c007377dd8140daeed549d3838930/tsconfig.json#L6

I prefer es5 mostly, but I set es3 because your code style is old.

---

Why are there still space indents????

Did you really review the code? I included min.js code as well, which is generated automatically.

https://github.com/wf9a5m75/lz-string/blob/f342fe51bd5c007377dd8140daeed549d3838930/libs/lz-string.min.js

---

Most important thing is getting some decent code quality in via tslint

You should point out where.

---

If you don't want to accept this PR, I don't mind. Just close this. I will distribute as lz-string typescript version.

[wf9a5m75]

I add some comments for if you are not familiar with TypeScript, just in case.

The libs directory is now generated by TypeScript. Checking libs/lz-string.js does not make sense anything.

Your code moved to src/lz-string.ts. https://github.com/pieroxy/lz-string/blob/f342fe51bd5c007377dd8140daeed549d3838930/src/lz-string.ts

I copied your code, and modified for TypeScript and tslint, but I keep the original code as much as possible.

When you need to change the code, you need to modify the code of src/lz-string.ts. Then execute npm run build. libs directory will be generated automatically.

---

The code passes tslint checking, jest check, your tests folder check. What else do you need?

Should I include travis CI or circleCI setting file?

Please review more seriously.

[JobLeonard]

Why your code is written in es3?

This is a really old library, still being used in really ancient contexts. We'd like to keep that backwards compatibility working, which as some point will mean splitting it into a modern and a legacy version.

[wf9a5m75]

Yes, I understand. That's why I set es3.

[wf9a5m75]

I will add travis configuration file.

[wf9a5m75]

One more thing. I usually do not include compiled directory, which is libs directory in this case. Instead of that, I add npm run npmpub script, then distribute only necessary files. However, I left libs directory in order to keep backward compatibility.

[wf9a5m75]

https://travis-ci.org/wf9a5m75/lz-string/builds/451473967

[wf9a5m75]

Here you are. npm run test and npm run build automatically. https://travis-ci.org/wf9a5m75/lz-string/builds/451489009

[wf9a5m75]

Regarding of #47, using Map improves compress speed 2x. In order to keep backward compatibility, I wrap Map with getMapOrObject function.

const getMapOrObject = (): any => {
  return (typeof Map === "undefined") ? new BaseMap() : new Map();
};

https://github.com/wf9a5m75/lz-string/commit/e1ce6781960eaf834dc67266233497f3bb4efe20#diff-fde13a29341c70bbb3f10652fae42cbb

Of course, test passed. https://travis-ci.org/wf9a5m75/lz-string/builds/451518466

[Rycochet]

I've not got time to go over this properly (for several days at least) - I've been almost exclusively TS for years now, so will definitely do so when I can.

One important thing - if you're doing a plain conversion to TS, then make it an absolutely plain conversion - no changes such as adding Map - those deserve a separate PR of their own because it otherwise makes it a change rather than a re-implementation :-)

[wf9a5m75]

Github updates automatically when I update my repo. I understand your point. If you want to merge PR a plain conversion, please merge this commit. https://github.com/pieroxy/lz-string/tree/9c21a428a83a312483862b16a45074e3cef73dde

I wonder why I have to wait for you for several days even there are some contributors.

I don't mind if you don't merge this PR. I archived my purpose for my project at least.

[Rycochet]

You can create a branch on your own repo and create a PR from that - basic git usage.

I'm just very busy, and only one of several people - but potentially the one with the most Typescript experience. Even if I could look through this tomorrow (and I feel it's more than likely I'll not get enough time till December actually) - I'd still want at least one other person to look over it too - this is a library with a lot of users, and it can't afford to break compatibility on any otherwise obsolete browsers or NodeJS installs until we get enough time to do a full update (see other open issues for some of the ideas).

[wf9a5m75]

I understand the waiting time for testing a lot, not waiting one person.

Anyway, I leave this PR. Merging or not merging is depends on the contributors. I don't mind.

[JobLeonard]

We really appreciate the effort!

I'm sure it's OK (not enough TS experience to judge) and in the worst case you'll have created a good starting point! :)

(reminds me: do we have a list of contributors or something like that?)

[wf9a5m75]

TypeScript guarantees the compatibility of the code logic, even es3. If you doubt it, it means you doubt TypeScript itself. Since this library does not manipulate DOM elements at all, the generated JS file should work on all environments.

As far as I take a look inside the generated file, there is no possible code for any problem.

---

And if this library support Uint8Array, the minimum requirement should be es5 not es3, because Uint8Array is defined in es5 first.

---

Even if you want to test on multiple platforms with multiple browsers, you need to create a test project using cordova-paramedic with SauceLab.

The SauceLab provides multiple environments such as FireFox 4.

I created .travis.yml file with matrix in order to test the code on multiple platforms. I think testing on Node.js is enough, but if you don't agree, please define additional conditions (that's not my work).

[Rycochet]

@JobLeonard ...I don't think we do - definitely need a CONTRIBUTORS.md file!

@wf9a5m75 The testing does need looking at - there are occasional differences in browsers that mean even though NodeJS testing is good enough for almost everything, we do need to be able to test in browsers (via Travis / BrowserStack / locally etc) - I'm also a definite fan of having the test file next to the main fail - but that's probably another PR ;-)

Doh for Uint8Array - this is where Typescript can definitely help though (just need something like Babel with it I think) ;-)

[Rycochet]

Ok, after a long time not being able to / having the time to do anything, back at looking into this. @wf9a5m75 if you're still interested in continuing with this could you have a look at converting over to using https://github.com/formium/tsdx for a standard way of doing things - if not then are you happy for me to take over and change for that - shouldn't be any code changes, just lint and build changes - it also handles building for other platforms, and can easily add babel support at a later date :-)

[Rycochet]

Closed in favour of #174