Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Nodify, bundle and deprecate old files #4

Merged
merged 6 commits into from
Mar 9, 2017
Merged

Conversation

alshakero
Copy link
Collaborator

@alshakero alshakero commented Mar 3, 2017

Related to Palindrom/JSON-Patch-OT#5.

This is 100% backward compatible. Yet 100% Node and future Palindrom compatible.

Tested with Browser <script />, Node CommonJS, Babel ES6 imports and TS imports. All Jasmine tests pass on Node and browser.

With great Typings files, thanks to @tomalec awesome JSDoc.

Usage examples (tested each):

<script src='dist/json-patch-queue-synchronous.min.js'></script>

or

<script src='dist/json-patch-queue.min.js'></script>

in Node's CommonJS

var JSONPatchQueue = require('json-patch-queue').JSONPatchQueue;

Or

var JSONPatchQueue = require('json-patch-queue').JSONPatchQueueSynchronous;

in Node's TS and ES6

import { JSONPatchQueue } from 'json-patch-queue';

or

import { JSONPatchQueueSynchronous } from 'json-patch-queue';

@alshakero alshakero requested a review from tomalec March 3, 2017 13:03
Now tests run on Travis CLI instead of a virtual browser. Much faster
and cleaner.
@alshakero
Copy link
Collaborator Author

@tomalec ready for review.

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it looks kinde hacky that just ot be able to do https://github.com/Palindrom/JSON-Patch-Queue/pull/4/files#diff-eb50505eb60eb228148efeebce771b5f

Write

module.exports = JSONPatchQueue;
module.exports.default = JSONPatchQueue; 

instead of

if (typeof module !== "undefined") {
    module.exports = JSONPatchQueue;
}

We need to load 75lines = 28% code more in the browser.
https://github.com/Palindrom/JSON-Patch-Queue/pull/4/files#diff-8abfd87d68e18e03b76cb3f3591fc129

gruntfile.js Outdated
@@ -1,45 +0,0 @@
module.exports = function(grunt) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need bump task any longer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's two files that we need to bump. And I thought Grunt with Webpack is too much. They share a lot of the functionality.

@tomalec
Copy link
Member

tomalec commented Mar 8, 2017

I'd rather expect it to be:

  1. Plain source file without any webpack related stuff, that could be easily maintained and loaded as good old, non-bundled <script src='src/json-patch-queue.js'></script>
  2. module ready version that works with node, commonjs, babel, ts, liek <script src='dist/json-patch-queue.js'></script>, as this is the version which requires just 4 lines more, comparing to source
  3. Bundled version with all deps (not applicable here, as this one has none) ready for browsers

@alshakero
Copy link
Collaborator Author

@tomalec done. I tested again all the above tests and copied HTML spec runner to test minified files.

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would bump the version after merge, with single bump commit.
Other than that LGTM.

bower.json Outdated
@@ -1,6 +1,6 @@
{
"name": "json-patch-queue",
"version": "1.0.0",
"version": "2.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you already bumped the version

@tomalec
Copy link
Member

tomalec commented Mar 9, 2017

LGTM, please merge, and bump major version

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants