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

jsurl v2 #17

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

jsurl v2 #17

wants to merge 38 commits into from

Conversation

wmertens
Copy link

@wmertens wmertens commented Apr 1, 2017

UPDATE: This is on npm https://www.npmjs.com/package/@yaska-eu/jsurl2

I implemented more-or-less the grammar I describe in #16 (comment).

You can see some examples of what it produces in the test/jsurl2.js file. Notably, it now can handle dates too, and it leaves URI encoding to whatever uses it next, while being immune to it.

For backwards compatibility, the v1 could look at the string and see if it ends with ~, in which case it should call v2.

(I switched to AVA for tests because it makes iterative TDD so much nicer. I also removed the semicolons from the original tests via my formatter, apologies, I can figure out how to to put them back)

TODO:

  • decide which chars get escaped
  • implement escaping
  • implement .toJSON
  • document v2 API
  • adjust package
  • fix the CI testing, apparently AVA doesn't support Node 0.12 (but neither does nodejs.org)
  • JSON compatible: either is the same or fails to parse as JSON, can parse JSON
  • .toJSON() supported
  • Dates work like they do in JSON, except if setting the option rich: true
  • URI and Script chars are escaped

Has nice watch mode and output diffs
* shorter encoding
* uses only unreserved symbols, plus '*'
* fast encode/decode, uses more native functions
* does not encode against URI encoding, but is immune on decode
* supports Date objects
@bjouhier
Copy link
Member

bjouhier commented Apr 1, 2017

I'd like to discuss the specs more before accepting a PR. Can we continue the discussion on #16?

* prefix _ instead of - for special constants
* () for objects
* !~ for arrays
Very variable but gives an idea. v1 is twice as fast as v2 despite doing more work :( Needs profiling.
* not for final ~ representing `true` value
* switch parser to using string indexes instead of splitting into array, so the end-of-object can be distinguished from ~
v2 parse is faster than v1, but stringify is slower
It stringifies slower than v1 and JSON, but it parses faster than v1 and JSON :)
rewrote stringifier to keep intermediate results in array and replaced join('~') with smart join

performance.html:15 JSON: 200000 parsed in 731ms, 0.003655ms/item
performance.html:23 JSON: 200000 stringified in 448ms, 0.00224ms/item
performance.html:32 v1: 200000 parsed in 1337ms, 0.006685ms/item
performance.html:40 v1: 200000 stringified in 934ms, 0.00467ms/item
performance.html:49 v2: 200000 parsed in 601ms, 0.003005ms/item
performance.html:57 v2: 200000 stringified in 403ms, 0.002015ms/item
@wmertens
Copy link
Author

wmertens commented Apr 3, 2017

So here's what I think is left to do:

  • decide which chars get escaped
  • fix the CI testing, apparently AVA doesn't support Node 0.12 (but neither does nodejs.org)

Given that it is faster and smaller than JSON, I think I will use it to inject initial state for server-rendered SPA apps. To do so, it must be safe to put inside a '-delimited string in a <script> tag, which means that it should not have embedded newlines, \, ', < and perhaps others like NULL - but I'd like to retain the Unicode aspect.

@bjouhier
Copy link
Member

bjouhier commented Apr 4, 2017

Performance is impressive. Bravo!

I did not have time yesterday but I'll review tonight. I just have a few points:

  • You squashed the true value. A bit like HTML attributes but it would be more symmetrical to keep _T and _F and squash null. Also, null alone could be encoded as ~, couldn't it?
  • We should have a * encoding for all URL reserved characters. This is especially important for %, ?, #, &and = which delimit/encode query parameters in URLs. This way people won't get bad surprises if they misuse decodeURIComponent.
  • We need to bump the package version to 2.0, but maybe also change the NPM package name to jsurl2?

@bjouhier
Copy link
Member

bjouhier commented Apr 4, 2017

We can drop node 0.12 in unit tests BTW.

@wmertens
Copy link
Author

wmertens commented Apr 4, 2017

  • true: Well, I reasoned that true is a much more common value than null in our target data sets, and it makes an object-of-options pretty. Besides, since arrays end with a ~, it needs to be encoded there as _T~ anyway so symmetry doesn't work everywhere.
  • URL reserved set: Right, I'll pick some alpha characters for those.
  • v2: So I was thinking that in v2, you can still get the v1 encoder/decoder, but it would call v2 if the string ends with ~ (only works if short is false). Then the package would still be jsurl, but the v1 would be under jsurl/v1.
    That way, you only include the v1 code if you need it.
    Loading the package as a script in the browser, however, should load both JSURL and JSURL2.
    I think that way, you can retain the package name. However, either way is fine by me.

@bjouhier
Copy link
Member

bjouhier commented Apr 4, 2017

I have mixed feelings about the optimization on truevalues. I'd prefer _T everywhere (even at the expense of 2 extra chars) but I see your point.

I have another point: V1 was strictly aligned on JSON: jsurl.stringify(undefined) produced undefined (null inside arrays) and dates were converted to string. I'd like to keep this strict alignement, at least by default. For dates, I think that the _D encoding should be controlled by an option (which would be off by default). Main motivation is simplicity: basic JSURL API behaves like JSON and is compatible with v1; no special rules to learn.

For v1/v2 what about keeping the same package name but bumping the major version? Packages that depend on v1 will continue to get updates on v1. If your component only consumes feeds you can upgrade freely from 1.x to 2.x. but if it produces feeds consumed by other components you must check that the other components have been upgraded to 2.x.

@wmertens
Copy link
Author

wmertens commented Apr 6, 2017

Not yet had time, but found this: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON()_behavior

  • should also support toJSON

@bjouhier
Copy link
Member

bjouhier commented Apr 6, 2017

No problem. I'm a bit overloaded too.

I'm ok with the true optim. Don't change it.

And yes, toJSON() should be supported.
And .travis.yml should drop 0.x and bump to latest 4.x and 6.x.

[off-topic] Saw that you are also using vscode. I got upgraded twice today 😃 .

@wmertens
Copy link
Author

wmertens commented Apr 12, 2017

Alright:

  • .toJSON() supported
  • Dates work like they do in JSON, except if setting the option rich: true
  • URI and Script chars are escaped
  • Travis checks on v4 and latest stable Node

A few non-code todos left (see top)

[off-topic] Yes, vscode is awesome; the second release was because it ate my homework 😉

@wmertens
Copy link
Author

And now it imports v2 by default, v1 is at jsurl/v1. The package version is made v2.0.0 so the API breakage is clear.

Added documentation.

@bjouhier
Copy link
Member

@wmertens. I still did not find the time. But I won't have any excuse this week-end. Sorry for that.

@wmertens
Copy link
Author

wmertens commented Apr 28, 2017 via email

@bjouhier
Copy link
Member

bjouhier commented May 1, 2017

If I understand correctly you also want an option to make it js-literal-friendly.
That's ok for me but I see a risk of explosion of options. The real important point is that options only control stringify and parse can handle all options automatically (robustness principle), but I think we are already fully aligned on this.

Let me know when you are ready for merge. I'll do a detailed review then.

@yvele
Copy link

yvele commented Mar 20, 2018

Any progress on this?

And will it handle replacing ' with " as mentioned in #15 (comment) ?

@wmertens
Copy link
Author

@yvele yes it doesn't use '. You can try it out with npm i github:wmertens/jsurl#jsurl2

Comments welcome - I'm using it in production but I'm not storing anything long-term in it so I'm open to change encoding if necessary.

@wmertens
Copy link
Author

wmertens commented Sep 5, 2018

@bjouhier I did a bunch of cleaning up and making it more robust. Now it automatically parses JSON, v1 and v2, and it always outputs either valid JSON that decodes the same, or invalid JSON. Now it's a drop-in replacement for JSON.*, but it will ignore stringifiers/revivers.

I updated the README, focusing solely on v2 and how to use it.

The test fails because Jest doesn't work on Node v4 ;)

I think it's in good shape for a v2.0.0-rc1 release. It's a breaking change, but downwards compatible, so people can choose to stay on v1.

@wmertens
Copy link
Author

@bjouhier any thoughts on merging this? Otherwise I'm thinking to put this on npm as jsurl2…

Jest is no longer compatible with Node 4, plus that's no longer maintained anyway
@wmertens wmertens changed the title Attempt at Jsurl2 jsurl v2 Oct 31, 2018
Some browsers don't like it inside strings in JS
* change cmp so it accepts objects for round-trip encode and checking assertions
@wmertens
Copy link
Author

@bjouhier I have it on npm in a separate package, is that what you would prefer?

By making it a v2 release on Sage/jsurl it could help more people. The v2 would indicate that it's a breaking change.

@bripkens
Copy link

Awesome PR @wmertens. Exactly what I need just now 👍

@yannickoo
Copy link
Contributor

Oh almost two years have passed and there is still only 0.1.5 available on NPM. @bjouhier any chance we can push something forward here? 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants