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

Plans to deprecate -duplex and promote Proxy #151

Closed
tomalec opened this issue Feb 21, 2017 · 11 comments
Closed

Plans to deprecate -duplex and promote Proxy #151

tomalec opened this issue Feb 21, 2017 · 11 comments

Comments

@tomalec
Copy link
Collaborator

tomalec commented Feb 21, 2017

As a continuation to #96 with ready and promissing implementation at #138

We have draft a brief plan for the future of json-patch-duplex and new shiny Proxy version.
Here what we discussed with @warpech and @alshakero :

  1. Create a new repo for JSONPatchProxy/JSONProxyPatch in PuppetJS organization (to be renamed to Palindrom) as this is quite standalone thing which can do it's job good regardless of jsonpatch.apply implementation.
    It will most probably use fast-json-patch as dependency but any build, testing, (npm) packaging process would become independent in those two.
  2. In this repo, we will mark json-patch-duplex as deprecated/obsolated and suggest adopting to new JSONProxyPatch.
  3. We will modularize (npm+browserify+ES modules) the code, but keeping in mind fast implementation and development is our goal, not the super flexible and full coverage over all package managers.
  4. We will separate useful parts of -duplex like compare, etc. to be separate module/export but from this repo.
  5. Our use cases (Puppet/Palindrom) will switch to Proxy version, so will consume both fast-json-patch and jsonproxypatch as dependencies.
@alshakero
Copy link
Collaborator

@tomalec. Gold! Thank you for noting everything. By the end of this week most of this will be done.

@alshakero
Copy link
Collaborator

So if we agree with JSONProxyPatch, should I call the class JSONProxyPatch? As in new JSONProxyPatch()?

@warpech
Copy link
Collaborator

warpech commented Feb 22, 2017

So if we agree with JSONProxyPatch, should I call the class JSONProxyPatch? As in new JSONProxyPatch()?

This looks correct according to how most official APIs are named in ECMAScript (https://www.ecma-international.org/ecma-262/5.1/#sec-15.12)

@alshakero
Copy link
Collaborator

alshakero commented Feb 22, 2017

So here is the semi-final version. Don't worry about the naming, or the logo (2 mins, not mine).

What I did:

  1. Cleaned the code a little bit.
  2. Migrated proxy tests. Now they run on Browser and Node.
  3. Tested on Edge, Chrome, Safari, IE and FF. Only IE fails.
  4. Tested on Safari iOS, Chrome iOS, both work, but tests (not the lib) need perfecting.
  5. Packed for NPM.
  6. Tested with:
    • CommonJS includes var a = require('b')
    • With TS import import JSONPatcherProxy from 'jsonpatcherproxy';
    • With ES6 import import JSONPatcherProxy from 'jsonpatcherproxy';
  7. Added Travis config and badge, all specs pass.

Next steps:

  1. Fixing the six failing specs. And only because of Lodash's array isEqual. 100% not our fault.
  2. Maybe renaming, I hope not though.
  3. Pushing to the right repo.

@warpech
Copy link
Collaborator

warpech commented Feb 22, 2017

Pushing to the right repo.

No need for that, you can just transfer the ownership to the right org.

@alshakero
Copy link
Collaborator

alshakero commented Feb 23, 2017

Update:

  • All tests now pass on Edge, Firefox, Chrome and Opera. And on mobile they pass on Safari iOS and Chrome iOS.
  • TS types are legit now, with documentation and IntelliSense works great.

I think this is done so far. I'll move to renaming PuppetJS.

@alshakero
Copy link
Collaborator

alshakero commented Feb 23, 2017

Transferred to Palindrom org.

@alshakero
Copy link
Collaborator

@tomalec should we start deprecating and promoting proxies? And will Palindrom 3.0.0 use them?

@warpech
Copy link
Collaborator

warpech commented Apr 26, 2017

Update: Patch generation using ES6 Proxy was implemented as a separate package: https://github.com/Palindrom/JSONPatcherProxy

In this repo we have a dirty-checking based patch generation, but it will be deprecated in future.

@felixfbecker
Copy link
Contributor

  • We will modularize (npm+browserify+ES modules) the code, but keeping in mind fast implementation and development is our goal, not the super flexible and full coverage over all package managers.
  • We will separate useful parts of -duplex like compare, etc. to be separate module/export but from this repo.

yes please. The code duplication between the duplex and normal version is horrible for making changes.

@alshakero
Copy link
Collaborator

alshakero commented Jun 2, 2018

I'll close this as irrelevant anymore. It doesn't sound like a great idea to kill IE support and observing objects you don't own support, while it's costing us next to nothing sitting here. It looks to me those two libs won't be merged anytime soon.

Unless we decide to invest in version 3.0 with a different API (that allows for internal proxies) because this one doesn't. Because it doesn't assume object ownership.

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

No branches or pull requests

4 participants