search

pieroxy / lz-string

LZ-based compression algorithm for JavaScript
MIT License
4.01k stars 565 forks source link

LZ-string 2.0 and preceding RC #117

Open JobLeonard opened 6 years ago

JobLeonard commented 6 years ago

So let's decide once and for all what we need to do to merge that pull request

Here's a TODO list with some first thoughts, I'll update it as the discussion evolves. It's very elaborate, but that's just because I like to keep it fine-grained.

Decide what to include

Suggestions:

Test for bugs

Benchmarks

Have a release candidate alongside current version for to find bugs

For how long, and how do we do this on npm?

Rycochet commented 6 years ago

ES6 version should simply be built in parallel so it's available on npm - just like the Typescript typings, if it's there then things that understand it can make use of it, just need to decide on the exact source structure to build that (here I'm leaning towards Typescript like always - until something like that I'd suggest just leaving it alone) ;-)

I'd be tempted to keep a v1 branch so anything important that comes up can be done without having "that" PR included ;-)

I'd suggest all development on / against a develop branch, and only merge to master when we're happy about tests passing and bugs fixed - along with the (semi-)standard "one branch per fix" idea (instead of developing directly on develop) - then we have several here now that can review PRs when a branch is ready to merge back into develop.

Apart from that I'm happy with all your suggestions there.

As the code is identical browser and Node, checking for correctness can be done headless (ie, the old "compress, decompress, compare" style) - so choosing a selection of files to check against, including true binary such as photographs. It might be worth collecting various files for the tests and either including them directly in the project, or including them in another branch / project - there may already be such a project available if someone wants to look (Octonode is good for grabbing Github content).

Side effect of that - being able to write the tests to be run on TravisCI etc, and have them report back to any PRs - it might not be the full in-all-browsers test without extra work, but it should have identical results for correctness.

JobLeonard commented 6 years ago

I made an ES6 version, in the minified benchmarks it's faster in Chrome, but a lot slower in Firefox for some reason:

https://run.perf.zone/view/LZString-variants-11-07-20182018-minified-compress-documentbodyinnerHTML-1531579254745

I already tried if it makes a difference whether we use function(){} or ()=>{} syntax (since it has different binding semantics), but this does not seem to be the case. Which means that Map is the likely culprit, weirdly enough.

https://run.perf.zone/view/LZString-variants-14-07-2018-minified-compress-documentbodyinnerHTML-1531579848737

JobLeonard commented 6 years ago

As the code is identical browser and Node, checking for correctness can be done headless (ie, the old "compress, decompress, compare" style) - so choosing a selection of files to check against, including true binary such as photographs. It might be worth collecting various files for the tests and either including them directly in the project, or including them in another branch / project - there may already be such a project available if someone wants to look (Octonode is good for grabbing Github content).

Sounds great. However, LZ-string 1 is backwards compatible all the way to IE8, maybe even IE6. How would we test for that?

JobLeonard commented 6 years ago

Test whether the bottleneck is the call to Map.has(). Not quite, but the alternative is faster on both Chrome and Firefox:

https://run.perf.zone/view/LZString-variants-14-07-2018-minified-compress-documentbodyinnerHTML-no-Maphas-1531582559384

Rycochet commented 6 years ago

Is that ES6 versions tree-shakeable? That's really cool though - more browsers are supporting it all the time, plus useful for Webpack / Rollup :-)

The arrow function is meant to be slightly faster than a function due to it not being a fully-featured version (ie, they are different specs, and not just in binding) - if it's in something that's called a lot then I'd hope it'd be different, but I've not seen tests so apart from that snippet of information I have no opinions (and generally compile via Babel).

We can test via BrowserStack (they give free accounts for Open Source projects) - but I've not written automated tests for it before, plus not sure if it can report back to a Github PR - I've seen something via TravisCI that implies it can be setup, but would need to look into it when I have time. In the meantime I'd say just getting a pile of automated tests together, they can always be repackaged later - better to have comprehensive automated tests that indicate functionality breaks on any PR, and some manual tests for in-browser testing.

JobLeonard commented 6 years ago

Is that ES6 versions tree-shakeable? That's really cool though - more browsers are supporting it all the time, plus useful for Webpack / Rollup :-)

I tried making it use modern module syntax, but Jasmine trips up over that. However, changing it back into the old exports version it still fails a number of tests that work fine when tested manually in the console, so I might as well ignore that.

AllNamesRTaken commented 4 years ago

reanimation What happened?