Sync with Node.js master

[lukechilds]

Resolves #32

Obviously not ready to merge yet, just opening this PR now to have a central place for review and discussion regarding these changes. Ready to rock and roll!

---

Just to explain my thought process:

The initial commit in this branch is nuking all existing work and the second commit is setting up the project in the way discussed in the related issue. Two main reasons for this:

1. I thought it would be messy to have essentially two code bases co-existing in the same branch while we're working on this. I thought it would just add confusion and add a load of noise to commits.

2. If we decide at some point that actually this should be a new project in a separate repo, we can just push this branch to master in the new repo and delete all history before my first nuke commit. That would give us a completely clean repo with proper history.

---

I've included assert and all tests directly from current Node.js master, including a comment header of the commit hash it references in the Node.js repo.

I've also added a simple tape wrapper around the Node.js tests and these are running on Travis.

My general plan is:

• First work on getting the tests working (or as many as we reasonably can) against Node.js core assert/util modules without relying on internal Node.js functionality.
• Once tests are working, switch over to testing against our assert and fixing that to pass tests without relying on internal Node.js functionality.
• Once our assert is passing tests, switch over to testing against browserify util rather than Node.js util and fixing that to pass tests without relying on internal Node.js functionality.
• Once everything is working in pure userland code without relying on any internal functionality or Node.js core dependencies, we can transpile the resulting code down to something appropriate for our browser targets and shim any required APIs. Also add automated browser testing.

util is used in both tests and assert so it might not be possible to split the work up into clean atomic steps as described here but that's my general idea.

---

The diff is going to be pretty noisy due to the substantial changes to the project structure so if you want to get a quick overview just have a look at the tip of the branch: https://github.com/browserify/commonjs-assert/tree/sync-with-node-master

I'm just fleshing everything out in this branch for now. However, it'll probably make sense to switch to feature branches that get merged into this branch to track changes more easily and allow other people to contribute.

Does this project structure make sense? Does my plan make sense? Any suggestions?

// cc @goto-bus-stop @BridgeAR

[lukechilds]

Oh also @BridgeAR, is it ok to run the Node.js tests in this way? Serially requiring each test file in the same process?

Or does each test file expect to be run in it's own isolated Node.js process?

Also, what are those .out files for in test/pseudo-tty? Can I delete them?

[lukechilds]

Woop woop, tests are passing against Node.js v12.0.0-rc.1 on Travis 🎉

https://travis-ci.org/browserify/commonjs-assert/jobs/520670587

I'm guessing Node.js v11 is failing because the tests are testing functionality in master (and v12) that isn't in Node.js v11 core assert/util.

Hopefully these tests should start passing as I port assert/util over to pure userland code.

[gfx]

I've given it a try in my package with karma+karma-webpack (+ts-loader), and found its dependency "buffer" is incompatible with webpack/node-libs-browser's so my test doesn't pass with the default webpack nodejs polyfills.

This is, I think, a problem in node-libs-browser (webpack/node-libs-browser#68 issued in 2017!), but until the issue is fixed, "buffer": ">= 4" is better in the version spec.

---

(edited)

Here are my karma settings that use this branch: msgpack/msgpack-javascript#20
This doesn't work with the released versions of commonjs-assert because it depends on the latest NodeJS's assert behavior.

[lukechilds]

We can probably actually remove the buffer dep here in favour of a simpler Buffer.compare shim.

[lukechilds]

Literally just pulled the compare shim back in that was already in master:

commonjs-assert/assert.js

Lines 5 to 37 in 8386769

	// compare and isBuffer taken from https://github.com/feross/buffer/blob/680e9e5e488f22aac27599a57dc844a6315928dd/index.js
	// original notice:
	/*!
	* The buffer module from node.js, for the browser.
	*
	* @author Feross Aboukhadijeh <[email protected]> <http://feross.org>
	* @license MIT
	*/
	function compare(a, b) {
	if (a === b) {
	return 0;
	}
	var x = a.length;
	var y = b.length;
	for (var i = 0, len = Math.min(x, y); i < len; ++i) {
	if (a[i] !== b[i]) {
	x = a[i];
	y = b[i];
	break;
	}
	}
	if (x < y) {
	return -1;
	}
	if (y < x) {
	return 1;
	}
	return 0;
	}

All unit tests are still passing.

[lukechilds]

@gfx just seen your edit

Here are my karma settings that use this branch: msgpack/msgpack-javascript#20
This doesn't work with the released versions of commonjs-assert because it depends on the latest NodeJS's assert behavior.

Just to clarify, are you saying your code doesn't work with the current published assert but does work with this branch?

[gfx]

Just clarify, are you saying your code doesn't work with the current published assert but does work with this branch?

Yes! So I am waiting for the PR is merged!

[lukechilds]

@goto-bus-stop @BridgeAR any ETA for publishing this?

Are you just holding off merging so you can do some SemVer minor releases first or do you need something else from me?

[goto-bus-stop]

it's unlikely i'll be able to take a close look at this very soon, so i'm happy to go with your and @BridgeAR's judgement

[lukechilds]

I'm happy with it and we've got a user report saying it's fixing issues for them so I'd say it's all good. I don't have npm permissions though so can't publish myself.

[gfx]

Is this a task to prepare npm publish? If so, you should use the prepare task:

https://docs.npmjs.com/misc/scripts#prepublish-and-prepare

So I can use this library by npm install browserify/commonjs-assert even in IE11, whereas other modern browsers does not require ES5 artifacts.

[lukechilds]

Added, thanks.

[goto-bus-stop]

@lukechilds added you

[lukechilds]

Thanks!

Ok unless I hear anything else from you @goto-bus-stop @BridgeAR I'll merge and do a new release tomorrow. 🎉

[kuraga]

YYYYYYYYYYYYEEEEEEEEEEEEESSSSSSSSSSSS!!!!!!!!!!!!!!!!!

[lukechilds]

Just published [email protected] with these changes!