Skip to content

Conversation

@macmenot
Copy link

@macmenot macmenot commented Jul 8, 2014

Presumably these cached dependencies shouldn't be committed in nodeunit vcs?

@mreinstein
Copy link
Collaborator

@caolan I'm assuming node_modules/ is checked in because nodeunit is an application and not a library. Is that fair to say?

@caolan
Copy link
Owner

caolan commented Sep 11, 2016

@mreinstein that may have been the reasoning at the time, but feel free to remove it if you like - I think most people would expect to have NPM pull in the dependencies now. Also, see the 'deps' directory - there are still some modules in there that probably date to a time before NPM was the clear package manager winner for node

@mreinstein
Copy link
Collaborator

I think most people would expect to have NPM pull in the dependencies now

@caolan if node_modules is removed from git and then is republished to npm, will that be a breaking API change? e.g., if a project depends on npm install -g nodeunit will that now break, or do globally installed modules have their deps installed implicitly? I guess I'm just wondering if this bumps the major semver number for nodeunit.

Also, see the 'deps' directory

What should be done with the modules in deps/ ? do these sound right?

  • deps/async.js - replace with async from npm
  • deps/ejs/ - replace with ejsfrom npm
  • deps/console.log.js - remove, no longer needed
  • deps/json2.js - remove, no longer needed

@caolan
Copy link
Owner

caolan commented Sep 12, 2016

@mreinstein globally installed modules will still have their dependencies installed by default. It's only a breaking change for people that reference the files directly inside their node_modules/nodeunit... which I'd say was an undocumented interface.

So I think a minor version bump is OK but it depends how strict you'd like to be.
As for the deps directory, your plan looks sensible. I suppose removing json2.js might remove support for very old browsers in the browser test runner, so that might require a major version bump.

@mreinstein
Copy link
Collaborator

mreinstein commented Sep 13, 2016

resolved via 4ff6c67 and f8e93e4

@mreinstein
Copy link
Collaborator

@caolan it looks like async and json2 are both part of the funky makefile setup. Not sure what should be done about these:

λ ag "deps/json2"
Makefile
30: cat deps/json2.js >> $(BUILDDIR)/browser/nodeunit.js
111:    cp deps/json2.js $(BUILDDIR)/commonjs/deps
λ ag "deps/async"
examples/browser/nodeunit.js
1460://    async = require('../deps/async'); //@REMOVE_LINE_FOR_BROWSER
1650://var async = require('../deps/async'), //@REMOVE_LINE_FOR_BROWSER

lib/nodeunit.js
11:var async = require('../deps/async'),

lib/core.js
16:var async    = require('../deps/async'), //@REMOVE_LINE_FOR_BROWSER

lib/reporters/junit.js
15:    async = require('../../deps/async'),

lib/types.js
17:    async = require('../deps/async'); //@REMOVE_LINE_FOR_BROWSER

lib/utils.js
11:var async = require('../deps/async'),

Makefile
37: cat deps/async.js >> $(BUILDDIR)/browser/nodeunit.js
112:    cp deps/async.js $(BUILDDIR)/commonjs/deps

test/test-base.js
13:    async = require('../deps/async'),       // @REMOVE_LINE_FOR_BROWSER

@caolan
Copy link
Owner

caolan commented Sep 13, 2016

heh, yeah I'd forgotten about that! the right solution, if you're up for it, is probably to drop the old makefile and use one of the more modern bundlers like browserify or webpack.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants