Skip to content

remove unneded nimnjs dependency, move opencollective to devDependenc…#59

Merged
amitguptagwl merged 2 commits intoNaturalIntelligence:masterfrom
Delagen:master
Feb 28, 2018
Merged

remove unneded nimnjs dependency, move opencollective to devDependenc…#59
amitguptagwl merged 2 commits intoNaturalIntelligence:masterfrom
Delagen:master

Conversation

@Delagen
Copy link
Contributor

@Delagen Delagen commented Feb 28, 2018

…ies and replace it with more light opencollective-postinstall

@Delagen
remove unneded nimnjs dependency, move opencollective to devDependenc…
d47aa71 
…ies and replace it with more light opencollective-postinstall
@Delagen
Copy link
Contributor Author

Delagen commented Feb 28, 2018

What do you think @amitguptagwl ?
opencollective ~14Mb, ~3500 files which is unneded in production
opencollective-postinstall ~4Mb, but I think too large also

@coveralls
Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage remained the same at 99.038% when pulling d47aa71 on Delagen:master into 5614beb on NaturalIntelligence:master.

3 similar comments
@coveralls
Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage remained the same at 99.038% when pulling d47aa71 on Delagen:master into 5614beb on NaturalIntelligence:master.

@coveralls
Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage remained the same at 99.038% when pulling d47aa71 on Delagen:master into 5614beb on NaturalIntelligence:master.

@coveralls
Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage remained the same at 99.038% when pulling d47aa71 on Delagen:master into 5614beb on NaturalIntelligence:master.

@coveralls
Copy link

coveralls commented Feb 28, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 99.038% when pulling 08f1cb9 on Delagen:master into 5614beb on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

amitguptagwl commented Feb 28, 2018 •
edited
Loading

Agree. However, I'm keeping the nimnjs as I'm doing some experiments which will be released soon, I hope it is small and will not cause any issue.

@amitguptagwl amitguptagwl merged commit 1a88910 into NaturalIntelligence:master Feb 28, 2018
@amitguptagwl
Copy link
Member

amitguptagwl commented Mar 1, 2018

@Delagen How did you figure out the size of opencollective?

du -hs node_modules/opencollective/
648K	node_modules/opencollective/

I even checked the size of node_modules before and after this patch. There is not much change in the size.

Moreover opencollective-postinstall is giving error at the time of installation while opencolletive is working fine.

@Delagen
Copy link
Contributor Author

Delagen commented Mar 1, 2018 •
edited
Loading

@amitguptagwl Install it in separate clean project. It does not big itself, but take a lot dependencies. It give error (command not found) only in prod due postinstall script, but finish successfully. In dev it give full message

npm i NaturalIntelligence/fast-xml-parser

> fast-xml-parser@3.2.3 postinstall C:\1\node_modules\fast-xml-parser
> opencollective-postinstall || exit 0

'opencollective-postinstall' is not recognized as an internal or external command,
operable program or batch file.
npm WARN saveError ENOENT: no such file or directory, open 'C:\1\package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open 'C:\1\package.json'
npm WARN 1 No description
npm WARN 1 No repository field.
npm WARN 1 No README data
npm WARN 1 No license field.

+ fast-xml-parser@3.2.3
added 2 packages from 6 contributors in 12.831s

In dev mode I see, may be revert to opencollective since it only dev dependency?

@Delagen
Copy link
Contributor Author

Delagen commented Mar 1, 2018

small patch

Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- package.json	(revision 5dfa8cdcd7d7eed47b39ee9ac4dc65e50cd11d43)
+++ package.json	(date 1519893974848)
@@ -9,7 +9,7 @@
     "bundle": "browserify src/parser.js -s parser > lib/parser.js",
     "coverage": "node ./benchmark/perfTest.js; istanbul cover -x \"spec/*spec.js\" jasmine spec/*spec.js;",
     "coverage:check": "istanbul check-coverage --branch 90 --statement 90",
-    "postinstall": "opencollective-postinstall || exit 0"
+    "postinstall": "opencollective postinstall || exit 0"
   },
   "bin": {
     "xml2js": "cli.js"
@@ -69,7 +69,7 @@
     "http-server": "^0.11.1",
     "istanbul": "^0.4.5",
     "jasmine": "^3.0.0",
-    "opencollective-postinstall": "^1.1.1",
+    "opencollective": "^1.0.3",
     "portfinder": "^1.0.13",
     "xml2js": "^0.4.19",
     "zombie": "^5.0.8"

@amitguptagwl
Copy link
Member

amitguptagwl commented Mar 1, 2018 •
edited
Loading

@Delagen

  1. apologies, I had to revert old commit as I had to fix and release remove unneded nimnjs dependency, move opencollective to devDependenc… #59.
  2. I checked with empty repo and I agree that there is a huge difference.
  3. Your suggestion to move it under devDependencies is a better approach.

I'll apply it manually and will publish along with the fix of #60 (if it is not invalid)

Happy Holi till that time

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Delagen @coveralls @amitguptagwl