Palindrom / JSON-Patch-OT

Operational Transformations for JSON Patch
MIT License
24 stars 0 forks source link

Publish on NPM #5

Closed alshakero closed 7 years ago

alshakero commented 7 years ago

This issue applies to JSON-Patch-OT, JSON-Patch-Queue, and JSON-Patch-OT-agent.

Because many people rely on these repos and use files in src directly. And because we learned the hard way with fast-json-patch that a Node module that supports TS and ES6 imports and runs in the browser without bundling is buggy (here: https://github.com/Starcounter-Jack/JSON-Patch/issues/140). My preliminary plan is to do the following:

1) Keep the files in src untouched, logic wise. And emit a warning asking people to use the bundle in dist folder. With the option to use a minified bundle. 2) Create a new, temporary dir, called newsrc, and put the Node friendly files there. Bundle them with webpack if needed, and put their compiled Browser + Node friendly versions in dist. And also make sure to throw a warning in these files to use dist folder in production. These files shouldn't be used at all except for development. Everything is in dist.

By the time all people start using dist folder, nobody will be using neither src or newsrc. And this will give us the freedom to remove the old files, and rename our new folder to src and live happliy ever after.

WDYT @tomalec @warpech?

warpech commented 7 years ago

Because many people

Who is this many people? Current Starcounter users? Or external community members?

I suppose that the community of people from outside of Starcounter who use JSON-Patch-OT, JSON-Patch-Queue, and JSON-Patch-OT-agent is only a fraction of the community of fast-json-patch users. So we don't have to care that much about smooth transition here.

My opinion is to just break the backwards compatibility and increment the major number in JSON-Patch-OT, JSON-Patch-Queue, and JSON-Patch-OT-agent.

@alshakero nevertheless I appreciate the community first approach, this should be our default consideration always.

@tomalec WDYT?

alshakero commented 7 years ago

@warpech well everywhere Puppet is used in Starcounter or otherwise, these repos are referenced. And I think it's a win-win situation. There is no harm at all of this transition. After one month we can simply delete src folder and rename newsrc to src without any problems and with a clean conscience, so to say 🤣

warpech commented 7 years ago

Starcounter comes with a copy of JSON-Patch-OT 1.0.1, so current Starcounter users do not care what we do in JSON-Patch-OT 2.0.0

Future Starcounter users also do not care what we do with JSON-Patch-OT 2.0.0, because it will be bundled in Palindrom 3.0.0

What's the fallacy in my reasoning?

alshakero commented 7 years ago

Hmmm you're right. I'll fix and test my PRs.

@tomalec if you agree, don't bother with reviewing for now.

warpech commented 7 years ago

Are the dist directories needed in JSON-Patch-OT, JSON-Patch-OT-agent, JSON-Patch-Queue and JSONPatcherProxy?

I was thinking that in these repos we can live with just the NPM way of defining exports. These libraries are unlikely to be used directly in a web browser.

alshakero commented 7 years ago

@warpech You're right. But these files need zero maintenance and serve a legit purpose.

warpech commented 7 years ago

But these files need zero maintenance and serve a legit purpose.

How come they are zero maintenance? Don't they require a building script? If I understand properly, if all we have is src and test, then we don't need any Babel or Webpack. If we have dist then we need Babel or Webpack.

alshakero commented 7 years ago

@warpech I honestly don't mind eitherway. I just think it's non-code work and all automated. If one person could use it, I wouldn't mind taking care of it. Especially that the work is done now and removing it to save time is actually time consuming.

If you think they're trouble, just thumbs up this and I'll remove them.

warpech commented 7 years ago

I that removing this is time saving in future, because there will be one piece less to worry about. Let's see what @tomalec has to say on this.

tomalec commented 7 years ago

runs in the browser without bundling is buggy

I wouldn't say running w/o bundling was buggy, as it was working perfectly for years. Lack of bundling was not the problem, the problems started mixing TS, NPM modules, and serve the same thing for the browser.


In general I do agree with @warpech. I also like to see clean repo with source files + spec only. That is the only think I am interested in, as a maintainer as well as a user of the module. Everything else is blurring the picture of what it is all about and complicating the development process.

Therefore, I would like to se the source fill as obfuscated as possible, and possible to be the same file I could load to the browser, or my node app. I think, few lines as those are quite fine to achieve that https://github.com/Palindrom/JSON-Patch-OT/blob/master/src/json-patch-ot.js#L152.

I don't think we should bother much about delivering minified files and I don't like using Webpack. If we will deliver just bare source file, consumers of this tiny module could still use any minifying/bundling/packing/browserifying/vulcanizing tool of their choosing. By using any of those, we are locking ourselves with it, adding maintenance overhead (even single cli call), get into the trouble of version of this tool, or migrating to new more fancy one.

During my overall Starcounter development, I would really like to be able to attach every single dependency as a non-minified file using separate <script> tag. It really helps to make the debugging more modular.

Maybe we could consider doing it similar to what Polymer team did some time ago: releasing just minified bundle of entire stack (in our case Palindorm+deps) in version bump commit - which was a outstanding leaf from master branch.

Or go even further and finally do such bundling on Stracounter level, so it would bundle entire Palindrom stack it uses, core CE definitions, and polyfills together, and by single flag I would be able to "unbundle" it for debugging.

tomalec commented 7 years ago

BTE, speaking of OP, @alshakero could you add me and @warpech as collaborators to all json patch packages you published https://www.npmjs.com/search?q=json+patch+ot ?

Maybe we could create Palindrom organization in NPM as well?

alshakero commented 7 years ago

Fair points. I'll take action.

could you add me and @warpech as collaborators

done

Maybe we could create Palindrom organization in NPM as well?

Then we'll have to pay.

tomalec commented 7 years ago

Then we'll have to pay.

For open-source? I thought it's free.

So maybe we could just create a Palindrom user? ;)

alshakero commented 7 years ago

@tomalec I didn't know but @warpech and I discussed it a month or so ago and he told me an org is paid and it would be smart to publish in individual accounts.

https://www.npmjs.com/org/create

tomalec commented 7 years ago

Yes, they are paid :( https://www.npmjs.com/pricing

warpech commented 7 years ago

"Organizations" is a paid plan (https://www.npmjs.com/pricing) that adds two features:

I think that we don't have need for any of the above.

Could you just add me and @tomalec as co-owners? https://docs.npmjs.com/cli/owner

alshakero commented 7 years ago

@warpech on it.

alshakero commented 7 years ago

@warpech turns out adding you as collaborators is equivilent to this. So you're owners already.

image

alshakero commented 7 years ago

@warpech I think this is good to close?

alshakero commented 7 years ago

Closing as all done.