Add the feature to update json2.js regulary from the upstream.

[junaruga]

Hi, I created a mechanism to update json2.js from the upstream easily. because I am inspired from uglifier's one. https://github.com/lautis/uglifier/blob/master/CONTRIBUTING.md

To deploy the upstream's json2.js to your package, you can run below commands.

git submodule update --init
bundle exec rake js

If you want to update json2.js, you can update submodule's commit hash.

I found you have updated the bundled json2.js by yourself. And I used the patch command to cover it.

The patch file is created by below command.

git format-patch -1 f47c02c8536ce7cf0850b6a511fcecad6539eeaf

It is this modification. https://github.com/rails/execjs/commit/f47c02c.patch

And its patch mechanism is inspired from nokogiri package. https://github.com/sparklemotion/nokogiri/tree/v1.6.8.rc3/patches/libxslt

I suppose that ideally you do not need to manage the patch by yourself. You can report the patch to the json2 upstream, https://github.com/douglascrockford/JSON-js and can use the fixed new json2.js.

How do you think?

I think that to use upstream's json2.js is better to use your bundled json2.js.

The reason why I created this feature is that I wanted to use system's json2.js file. I am managing your gem's RPM package in Fedora Project.

By the way, the json2.js is only used from JScript right now. And I could not run it by myself, because I do not have Windows environment.

Thanks.

[rails-bot]

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

[rafaelfranca]

Could you explain why this patch is necessary in execjs?

[rafaelfranca]

Oh, got it. I'll review carefully.

[junaruga]

Hi, @rafaelfranca

Could you explain why this patch is necessary in execjs? Oh, got it. I'll review carefully.

Thanks for your reviewing. Actually I could not understand why the patch [1] was necessary, that had been updated by someone in past time.

If I could understand it, I could send the json2.js upstream [2] a patch as a pull-request by myself. Unfortunately I do not have Windows environment to test JScript and reproduce the patch's issue.

[1] https://github.com/rails/execjs/commit/f47c02c [2] https://github.com/douglascrockford/JSON-js

[junaruga]

If you want to modify my pull-request, you can send me a pull-request to my repo's branch. [1] I will accept it.

[1] https://github.com/junaruga/execjs/tree/feature/update-json2-from-upstream-regulary

[junaruga]

@rafaelfranca

I checked why its patch was necessary at past time.

Conclusion

First of all, my conclusion is that we do not need the patch if all the versions of JScript does not include JSON object in the engine. However it is much better to update json2.js to latest version because that the patch's issue is fixed on latest json2.js.

Why? (Process)

I checked the past source code for the patch

Subject: [PATCH] Ensure JSON var isn't shadowed in JSC and Spidermonkey

git checkout f47c02c8536ce7cf0850b6a511fcecad6539eeaf

I could find that json2.js was used in external_runtime.rb#compile at that time. [1] Spidermonkey was defined as ExternalRuntime. That means Spidermonkey used json2.js at that time. [2] When the compiled js code on the Spidermonky, had "#{json2_source}" string inside of the code, json2.js was loaded.

Spidermonkey had JSON object in the engine at that time, such as curent Node.js [3]

And that means that json2.js's JSON object did override Spidermonky's JSON object before the patch was run, because of https://github.com/rails/execjs/commit/f47c02c

L162 var JSON;

However the issue looks fixed on the JSON-js upstream [4].

So, I think we can update json2.js to latest version easily.

And latest execjs Only JScript looks using json2.js to use "JSON.stringify" in jscript_runner.js. So, if JScript supports JSON.stringify in the JS engine, we do not need to manage json2.js any more.

That is my thoughts.

[1] https://github.com/rails/execjs/blob/f47c02c8536ce7cf0850b6a511fcecad6539eeaf/lib/execjs/external_runtime.rb#L43 [2] https://github.com/rails/execjs/blob/f47c02c8536ce7cf0850b6a511fcecad6539eeaf/lib/execjs/runtimes.rb#L22 [3] https://github.com/artyyouth/SpiderMonkey_JSON Back porting native JSON support from SpiderMonkey 1.85 to SpiderMonkey 1.70. => That means SpiderMonkey had JSON object at past time. [4] https://github.com/douglascrockford/JSON-js/commit/43d7836c8ec9b31a02a31ae0c400bdae04d3650d

[junaruga]

@rafaelfranca How was that?