Remove unnecessary dependencies

[adwydman]

I removed several dependencies from package.json:

• Set - supported natively in 4.4.7, unused
• require-dir - unused
• sealious-datastore-tingo - usused
• sha1 - unused
• require-self - unused

Also merge is not needed anymore, becauseObject.assign does pretty much the same thing (supported in 4.4.7). Four files use merge:

• config-manager.js - I rewrote merge to Object.assign here, because we have tests
• resource-type.js
• resource-type-collection-subject.js
• resource-type-field-subject.js - all of the three are not changed, because we don't have tests there.

[kuba-orlik]

1. require-self is necessary for require("sealious") to work. Please remove all require("sealious") instances and replace them with proper locreq calls to specific modules. require-self is a module that is being called during the prepublish step of the npm install process. See package.json:8 and the build status.
2. sealious-datastore-tingo is used. It is the default datastore. It might not be explicitly required, but Sealious does load it during init time, thanks to the plugin-manager magic. I brought it back to the package.json file.

3. as for the merge module: I appreciate the elegance of Object.assign, I didn't know such functionality is supported natively by Node.js. But merge.recursive and Object.assign's behaviors are different, as demonstrated by this example:

   > var objA = {foo: {bar: "baz", bat: "bax"}};
   > var objB = {foo: {bam: "bak"}};
   
   > merge.recursive(true, objA, objB)
   { foo: { bar: 'baz', bat: 'bax', bam: 'bak' } }
   > Object.assign({}, objA, objB)
   { foo: { bam: 'bak' } }

   Please revert the Object.assign changes.

PS. remember to git pull --rebase remove_unnecessary_dependencies, as I pushed two commits to the branch. (Note the --rebase! :) )

[adwydman]

Ad 1, Ad 2: Noted.

Ad 3: Uhm, I think your example is messed up,

> var objA = {foo: {bar: "baz", bat: "bax"}};
> var objA = {foo: {bar: "baz", bat: "bax"}};

These objects are the same.

[kuba-orlik]

Yep, sorry, I messed copy&paste :D I've updated my previous comment :)

[adwydman]

Ok, I see it now. Object.assign overwrites properties to the latest occurance.

[adwydman]

Btw, @kuba-orlik did you assign yourself to this PR purposefully?

[kuba-orlik]

Yes, as I'm the one who reviews it and will merge it :)

[adwydman]

Cool :)

[coveralls]

Coverage decreased (-1.7%) to 51.115% when pulling e3103a5a7b4ef211fd5b005116c236e6fdf25930 on remove_unnecessary_dependencies into e41dca416a2cafb5cd104c8316b13fc42f77fca4 on references.

[kuba-orlik]

What I find curious is that the unit tests didn't detect a change in behavior from merge.recursive to Object.assign. Could you add a test or two so they do take that into consideration? :)

[adwydman]

Because I used only one level of object nesting, so there was no difference between recursive merging and Object.assign.

2016-08-03 19:09 GMT+02:00 Kuba Orlik notifications@github.com:

What I find curious is that the unit tests didn't detect a change in behavior from merge.recursive to Object.assign. Could you add a test or two so they do take that into consideration? :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sealcode/sealious/pull/286#issuecomment-237287459, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgB1hGCPUzy358h1DQHBPlQTlaZFRgiks5qcMtRgaJpZM4JbaGa .

[coveralls]

Coverage decreased (-1.7%) to 51.115% when pulling a5c6b2931e2f41bd55dd937d4ba8f323ac23896a on remove_unnecessary_dependencies into e41dca416a2cafb5cd104c8316b13fc42f77fca4 on references.

[coveralls]

Coverage decreased (-1.7%) to 51.115% when pulling 745f4833a36de7f29ea5df123114d4f6d731d5ec on remove_unnecessary_dependencies into e41dca416a2cafb5cd104c8316b13fc42f77fca4 on references.

[coveralls]

Coverage decreased (-0.2%) to 51.115% when pulling 369e5384a0a3cc1663c37cab6a1971c59406843b on remove_unnecessary_dependencies into 38db39b7c710ee2cc9722ac93f72296aa7c0c449 on references.

[adwydman]

@kuba-orlik: I've added tests to references that won't pass if Object.assign is used:

https://github.com/sealcode/sealious/commit/7f82bba2a510928ac5b20030c985f6c724dfd983 https://github.com/sealcode/sealious/commit/f3c95dcb9ed4625281d853f7178ef22040ef3499

[kuba-orlik]

With require-self removed, require("sealious") will not work. In the remove_unnecessary_dependencies branch there are still many instances of require("sealious"). I will merge it once they are all gone :)

Here's a list of files that still require("sealious"):

lib/utils/run-action.js
lib/plugins/plugin-manager.js
lib/core-services/file-manager.js
lib/base-chips/field-types/file.js
lib/base-chips/field-types/single_reference.js
lib/base-chips/field-types/username.js
lib/base-chips/access-strategy-types/super.js
lib/base-chips/access-strategy-types/themselves.js
lib/base-chips/access-strategy-types/or.js
lib/base-chips/access-strategy-types/not.js
lib/base-chips/access-strategy-types/and.js
lib/base-chips/resource_type.user.js
lib/action.js
tests/unit-tests/response/response.test.js
lib/context.js
tests/unit-tests/base-chips/field-types/file.test.js

[adwydman]

There's a separate branch for that: https://github.com/sealcode/sealious/tree/remove_require_sealious :)

[kuba-orlik]

I know :) I hope you see that I can't merge this one unless the other one is finished.

[adwydman]

There are no require('sealious') there. Do you want me to merge that branch with this one?

[kuba-orlik]

Correct my method if it's wrong, but this is the way I check whether or not there are require("sealious") instances in this branch:

kuba@laptop:~/projects/sealcode/sealious/sealious$ git checkout remove_unnecessary_dependencies 
Switched to branch 'remove_unnecessary_dependencies'
Your branch is up-to-date with 'origin/remove_unnecessary_dependencies'.
kuba@laptop:~/projects/sealcode/sealious/sealious$ git pull
Already up-to-date.
kuba@laptop:~/projects/sealcode/sealious/sealious$ ag 'require\("sealious"\)' 
lib/utils/run-action.js
2:const Sealious = require("sealious");

lib/plugins/plugin-manager.js
2:const Sealious = require("sealious");

lib/core-services/file-manager.js
7:const Sealious = require("sealious");

lib/context.js
52:     const Sealious = require("sealious");
73: const Sealious = require("sealious");

lib/base-chips/field-types/file.js
2:const Sealious = require("sealious");

lib/base-chips/field-types/single_reference.js
2:const Sealious = require("sealious");

lib/base-chips/field-types/username.js
3:const Sealious = require("sealious");

lib/base-chips/access-strategy-types/themselves.js
2:const Sealious = require("sealious");

lib/base-chips/access-strategy-types/or.js
2:const Sealious = require("sealious");

lib/base-chips/access-strategy-types/super.js
2:const Sealious = require("sealious");

lib/base-chips/access-strategy-types/not.js
2:const Sealious = require("sealious");

lib/base-chips/access-strategy-types/and.js
2:const Sealious = require("sealious");

lib/base-chips/resource_type.user.js
2:const Sealious = require("sealious");

lib/action.js
2:const Sealious = require("sealious");

tests/unit-tests/response/response.test.js
4:const Sealious = require("sealious");

tests/unit-tests/base-chips/field-types/file.test.js
4:const Sealious = require("sealious");

[kuba-orlik]

(Maybe you meant to merge the remove_require_sealious branch to this one? :) )

[adwydman]

You are wrong, because you are checking remove_unnecessary_dependencies and not remove_require_sealious ;)

[kuba-orlik]

But... that's what this PR is for, right? The remove_unnecessary_dependencies branch :)

So I thought the request was: "Please merge remove_unnecessary_dependencies branch into references". And I cannot, because it's not ready. If you merge remove_require_sealious into remove_unnecessary_dependencies, or create a separate PR for remove_require_sealious, that's different business.

Excuse me if I missed something in what you were communicating, but I was sure you were talking about the branch that this PR is about.

[adwydman]

Ok, miscommunication. I'll merge remove_require_sealious to this branch.

[adwydman]

Merged remove_require_sealious into this one.

[coveralls]

Coverage increased (+0.04%) to 51.345% when pulling b7b34ea5b6575a1d529e81cccf91380c806f9e79 on remove_unnecessary_dependencies into 38db39b7c710ee2cc9722ac93f72296aa7c0c449 on references.

[kuba-orlik]

Two more files left :)

tests/unit-tests/response/response.test.js:1
tests/unit-tests/base-chips/field-types/file.test.js:1

I don't think removing require("sealious") from them would break anything, hm?

[adwydman]

You won't know unless you try it. :)

[coveralls]

Coverage decreased (-6.7%) to 44.798% when pulling 5a82648d8cb8f2d1a7990c22d64da387d2cb9652 on remove_unnecessary_dependencies into 421e50951d0b920f603fdd410479f31cf295d558 on references.

[kuba-orlik]

Integration tests fail on this branch, but pass on references.

They fail here because File Manager receives only the datastore type, not the actual datastore instance - here lies the proof. @adwydman Will you look into that? :)

[adwydman]

@kuba-orlik: The tests pass on Travis, but it also executes node . which fails.

[kuba-orlik]

node . runs integration tests ;)

[adwydman]

Okay! But I don't know how to instantiate Datastore without using require('sealious').

[kuba-orlik]

Hmm we could make Datastore a part of context, what do you think about that?

[kuba-orlik]

https://github.com/sealcode/sealious/blob/no_accept_reject_interface/lib/subject/subject-types/resource-type-collection-subject.js#L44 :)