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:

• [x] 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.
• [x] Once tests are working, switch over to testing against our assert and fixing that to pass tests without relying on internal Node.js functionality.
• [x] 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.
• [x] 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.

[goto-bus-stop]

per the email, it's true that replicating the exact behaviour is going to be a big rabbit hole. For the core comparison stuff, we should get as close as possible, but for the other things I think we should get close if it's easy to do but otherwise just aim for similar behaviour in everyday use. for errors for example, we could have something like:

var err = new Error('the message (not necessarily identical to Node\'s)')
err.code = 'THE_ERROR_CODE'
throw err

if the error code matches, the rest is less important. to align closer with Node it could be:

// errors.js
function ERR_INVALID_ARGUMENT (xyz) {
  var err = new TypeError('build the message etc ' + xyz)
  err.code = 'ERR_INVALID_ARGUMENT'
  return err
}
// elsewhere
throw new errors.ERR_INVALID_ARGUMENT('stuff')

[lukechilds]

Thanks very much for your feedback @BridgeAR!

12/13 tests passing in Node.js v12 🎉

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

[lukechilds]

I did see that implementation in Node.js core but saw that Reflect is not supported in IE so we'll need to polyfill Reflect anyway.

Is there any functional difference between them or is it purely syntax?

[BridgeAR]

@lukechilds there are some nuances that are different (it is harder to tamper with it. If the Function prototype was messed up it would still work) but otherwise works the same.

[BridgeAR]

The current failure seems to be that acorn is not imported. The question is if that should be imported or not.

[lukechilds]

Current version on npm (acorn@6.1.1) is 88.9kB minified / 27.3 kB minified + gzipped.

https://bundlephobia.com/result?p=acorn@6.1.1

@goto-bus-stop You seemed against this before. Do you think this would bloat assert too much?

[goto-bus-stop]

yeah. AFAICT acorn is only used in the code path that displays the actual source code in the error message, which I don't think we can do anyway.

[BridgeAR]

@goto-bus-stop it is indeed only used for that. Not everything from acorn is required, so I guess the overhead could further be reduced but it's definitely not required in all browsers. It is possible to get the actual source code in all Chromium based browsers but that is a different approach than the current one and requires fine tuning which would take some more time (and not a lot of people actually know how about the functionality in the first place).

It would also be possible to implement that without the use of acorn at least for common cases. acorn is used to guarantee that all code, no matter how complex or spanned over multiple lines would properly be outlined.

[lukechilds]

🎉 100% userland assert with all tests passing:

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

I've just added acorn/acorn-walk deps for now to complete the functionality. Happy to remove these and change tests to not require them before merging, but it's giving me a passing test suite so I can focus on other things for now.

Still depending on Node.js core buffer/util/string_decoder modules, but all assert code is 100% userland. Looks like existing browserify modules for buffer/string_decoder can be dropped straight in. As discussed, util will require some work.

[lukechilds]

@goto-bus-stop just a thought, how should we test IE support in assert without Promises?

Normally I'd just transpile with babel. Then if it's an application include babel-polyfill which will setup global Promise. If it's a library leave out babel-polyfill and let the end users include it.

However we need Promises to be able to run the tests in IE. We could load a Promise polyfill in the IE tests but that just seems wrong. I guess it will need a Promise polyfill to work in IE so maybe that's ok.

We could bundle Promise shims into assert locally, not affecting the global namespace, but then that's adding a load of bloat to the majority of targets that don't need it.

What's the best way to approach this?

[lukechilds]

Thanks for the feeedback @BridgeAR!

test/common/fixtures.js test/fixtures/assert-first-line.js test/fixtures/assert-long-line.js

These files are all used in: https://github.com/browserify/commonjs-assert/blob/1943c341d6c72772e2138148bc6d283d2ac21e40/test/parallel/test-assert-first-line.js

Or are you saying if I no longer testing the error message then I can safely remove that test (and all associated files)?

[lukechilds]

We've got a working browser assert!

Still need to test in other browsers, but nearly there! 🎉

[lukechilds]

Tests are now passing in Chrome, Firefox and Safari.

I've added a transpilation build step so I can start testing in IE. However after transpiling some tests are now failing in Node.js.

https://travis-ci.org/browserify/commonjs-assert/builds/527346644

They seem to be type based fails:

TypeError: The "promiseFn" argument must be one of type Function or Promise. Received type object

TypeError: The "fn" argument must be of type Function. Received type string.

Any idea how babel could have introduced this new behaviour?

I've tried logging around the relevant code that seems to be erroring and even replacing certain transpiled files with the originals and I'm still getting the errors. I'm not quite sure what's causing them.

[lukechilds]

const presets = [
  [
    "@babel/env",
    {
      targets: {
        edge: "44",
        firefox: "66",
        chrome: "73",
        safari: "12",
        // ie: "9"
      }
    },
  ],
];

module.exports = {presets};

This babel.config.js works without breaking tests, just disabling the IE transforms. Although I guess that's not that unexpected. There probably aren't a whole lot of transforms required for the other targets.

[lukechilds]

Ok, I've narrowed the babel issues down to makeNodeErrorWithCode:

https://github.com/browserify/commonjs-assert/blob/3e5c87bbb0b0f0df1b43852307c5e8041d3eb540/internal/errors.js#L30-L69

Source version:

function makeNodeErrorWithCode(Base, key) {
  return class NodeError extends Base {
    constructor(...args) {
      if (excludedStackFn === undefined) {
        super();
      } else {
        const limit = Error.stackTraceLimit;
        Error.stackTraceLimit = 0;
        super();
        // Reset the limit and setting the name property.
        Error.stackTraceLimit = limit;
      }
      const message = getMessage(key, args, this);
      Object.defineProperty(this, 'message', {
        value: message,
        enumerable: false,
        writable: true,
        configurable: true
      });
      addCodeToName(this, super.name, key);
    }

    get code() {
      return key;
    }

    set code(value) {
      defineProperty(this, 'code', {
        configurable: true,
        enumerable: true,
        value,
        writable: true
      });
    }

    toString() {
      return `${this.name} [${key}]: ${this.message}`;
    }
  };
}

is getting transpiled down to:

"use strict";

function _instanceof(left, right) { if (right != null && typeof Symbol !== "undefined" && right[Symbol.hasInstance]) { return right[Symbol.hasInstance](left); } else { return left instanceof right; } }

function _typeof(obj) { if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { _typeof = function _typeof(obj) { return typeof obj; }; } else { _typeof = function _typeof(obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj; }; } return _typeof(obj); }

function _classCallCheck(instance, Constructor) { if (!_instanceof(instance, Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } }

function _createClass(Constructor, protoProps, staticProps) { if (protoProps) _defineProperties(Constructor.prototype, protoProps); if (staticProps) _defineProperties(Constructor, staticProps); return Constructor; }

function _possibleConstructorReturn(self, call) { if (call && (_typeof(call) === "object" || typeof call === "function")) { return call; } return _assertThisInitialized(self); }

function _assertThisInitialized(self) { if (self === void 0) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return self; }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function"); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, writable: true, configurable: true } }); if (superClass) _setPrototypeOf(subClass, superClass); }

function _setPrototypeOf(o, p) { _setPrototypeOf = Object.setPrototypeOf || function _setPrototypeOf(o, p) { o.__proto__ = p; return o; }; return _setPrototypeOf(o, p); }

function _get(target, property, receiver) { if (typeof Reflect !== "undefined" && Reflect.get) { _get = Reflect.get; } else { _get = function _get(target, property, receiver) { var base = _superPropBase(target, property); if (!base) return; var desc = Object.getOwnPropertyDescriptor(base, property); if (desc.get) { return desc.get.call(receiver); } return desc.value; }; } return _get(target, property, receiver || target); }

function _superPropBase(object, property) { while (!Object.prototype.hasOwnProperty.call(object, property)) { object = _getPrototypeOf(object); if (object === null) break; } return object; }

function _getPrototypeOf(o) { _getPrototypeOf = Object.setPrototypeOf ? Object.getPrototypeOf : function _getPrototypeOf(o) { return o.__proto__ || Object.getPrototypeOf(o); }; return _getPrototypeOf(o); }

function makeNodeErrorWithCode(Base, key) {
  return (
    /*#__PURE__*/
    function (_Base) {
      _inherits(NodeError, _Base);

      function NodeError() {
        var _this;

        _classCallCheck(this, NodeError);

        if (excludedStackFn === undefined) {
          _this = _possibleConstructorReturn(this, _getPrototypeOf(NodeError).call(this));
        } else {
          var limit = Error.stackTraceLimit;
          Error.stackTraceLimit = 0;
          _this = _possibleConstructorReturn(this, _getPrototypeOf(NodeError).call(this)); // Reset the limit and setting the name property.

          Error.stackTraceLimit = limit;
        }

        for (var _len = arguments.length, args = new Array(_len), _key = 0; _key < _len; _key++) {
          args[_key] = arguments[_key];
        }

        var message = getMessage(key, args, _assertThisInitialized(_this));
        Object.defineProperty(_assertThisInitialized(_this), 'message', {
          value: message,
          enumerable: false,
          writable: true,
          configurable: true
        });
        addCodeToName(_assertThisInitialized(_this), _get(_getPrototypeOf(NodeError.prototype), "name", _assertThisInitialized(_this)), key);
        return _possibleConstructorReturn(_this);
      }

      _createClass(NodeError, [{
        key: "toString",
        value: function toString() {
          return "".concat(this.name, " [").concat(key, "]: ").concat(this.message);
        }
      }, {
        key: "code",
        get: function get() {
          return key;
        },
        set: function set(value) {
          defineProperty(this, 'code', {
            configurable: true,
            enumerable: true,
            value: value,
            writable: true
          });
        }
      }]);

      return NodeError;
    }(Base)
  );
}

Swapping this transpiled code block out for the original source stops all the errors.

Any ideas what's going wrong in the transpilation and how I can make a functionally accurate version of makeNodeErrorWithCode?

[lukechilds]

I tried implementing the internal errors in the same way as readable-stream.

The same tests fail but with different error messages. Even without transpilation, just testing directly against the source files.

[lukechilds]

Ok, I narrowed it down to this code block:

class NodeError extends Base {
  constructor (arg1, arg2, arg3) {
    super(getMessage(arg1, arg2, arg3));
  }
}

transpiling to:

var NodeError =
/*#__PURE__*/
function (_Base) {
  _inherits(NodeError, _Base);

  function NodeError(arg1, arg2, arg3) {
    _classCallCheck(this, NodeError);

    return _possibleConstructorReturn(this, _getPrototypeOf(NodeError).call(this, getMessage(arg1, arg2, arg3)));
  }

  return NodeError;
}(Base);

Seems like there are known issues extending native classes with babel:

https://babeljs.io/docs/en/babel-plugin-transform-classes#caveats

[lukechilds]

@BridgeAR https://github.com/browserify/commonjs-assert/pull/44/commits/f4c09e2caf507f5af47ea5dba2d78e393eacf74b is required for shimming Object.getOwnPropertyDescriptors in Node.js <7 and IE. However it massively slows down the test-assert-checktag.js test (a few minutes) and sometimes even crashes Node.js:

TAP version 13
# test-assert-async.js
ok 1 should not throw
ok 2 should not resolve to rejected Promise
# test-assert-checktag.js

<--- Last few GCs --->

[26640:0x103800c00]   113083 ms: Mark-sweep 1065.4 (1104.8) -> 1065.4 (1073.3) MB, 126.8 / 0.0 ms  (average mu = 0.429, current mu = 0.000) last resort GC in old space requested
[26640:0x103800c00]   113211 ms: Mark-sweep 1065.4 (1073.3) -> 1065.4 (1073.3) MB, 127.4 / 0.0 ms  (average mu = 0.261, current mu = 0.000) last resort GC in old space requested

<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x1bde8fedbe3d]
Security context: 0x25fae4b894b9 <JSObject>
    1: DoJoin(aka DoJoin) [0x25fa498ee401] [native array.js:~87] [pc=0x1bde901bfabd](this=0x25fa148826f1 <undefined>,l=0x25fa16e69199 <JSArray[1890458]>,m=1890458,A=0x25fa148828c9 <true>,w=0x25faab0f00a9 <String[1]\: \n>,v=0x25fa148829a1
<false>)
    2: Join(aka Join) [0x25fa498ee451] [native array.js:~112] [pc=0x1bde8fee20d8](this=0x25fa148826f1 <undefined>...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: 0x10003c597 node::Abort() [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 2: 0x10003c7a1 node::OnFatalError(char const*, char const*) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 3: 0x1001ad575 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 4: 0x100579242 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 5: 0x100582744 v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 6: 0x100554534 v8::internal::Factory::NewRawTwoByteString(int, v8::internal::PretenureFlag) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 7: 0x10082dd3d v8::internal::Runtime_StringBuilderJoin(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/lukechilds/.nvm/versions/node/v10.15.3/bin/node]
 8: 0x1bde8fedbe3d
/bin/sh: line 1: 26638 Abort trap: 6           npm run test:nobuild

Likely because that test loops over global and process which could be quite big.

Thoughts on what to do here?

[BridgeAR]

@ljharb please have a look at https://github.com/browserify/commonjs-assert/pull/44#issuecomment-489080189. This seems pretty bad and it sounds like there's something wrong going on.

@lukechilds the best solution is to replace all common.expectsError calls with assert.throws where it's not used as callback. It's almost identical with one mayor difference: the type property has to be replaced with name set to the class name. type checks for the class instance itself.

Another solution is to replace https://github.com/browserify/commonjs-assert/commit/f4c09e2caf507f5af47ea5dba2d78e393eacf74b#diff-9ae71bf1b8bff0e90c13e5b82ee79f09L220 with something like (untested but it should work):

if (typeof settings === 'object') {
  if (settings.name !== 'undefined' && settings.type !== undefined) {
    assert.throws(fn, innerFn);
  } else if (settings.type !== undefined) {
    const tmp = settings.type;
    delete settings.type;
    settings.name = tmp.name;
    assert.throws(fn, settings);
    settings.type = tmp;
  } else {
    assert.throws(fn, settings);
  }
} else {
  assert.throws(fn, innerFn);
}

[lukechilds]

Hmmn, for some reason object.getownpropertydescriptors is not slow in Node.js 6 and below.

It's 8 10 12 that are very slow.

And the shim is only needed for 6 and below.

So maybe it's ok to just only use the shim if we don't have native support?

[lukechilds]

Would be good to know what the issue was but I'm not experiencing any issues with https://github.com/browserify/commonjs-assert/pull/44/commits/1e2a19c283b915a42e49c9384583c0122cd20caa.

[lukechilds]

FWIW I'm running into the exact same perf/stability issues trying to shim new Map().

I don't think there's an issue with the object.getownpropertydescriptors shim (or Map shim). I think the issue is that we're swapping out fast native methods for slow userland methods in a recursive function.

Although it is strange that Node.js 6 and below can handle the slower versions. Node.js 8 and above are slow or crash.

If anything that seems like a Node.js issue though, not an issue with the shims.

[BridgeAR]

If I'm not mistaken it's intended to publish this as semver-major (which it should be). Thus, I would scratch support for IE < 11 and Node.js <= v6.

[kuraga]

Let's sync version number with NodeJs?

[lukechilds]

@BridgeAR Awesome, I've already tested it down to Node.js v0.12!

Oops, no I accidentally tested against 12, not 0.12. It's working down to Node.js 4 though.

[BridgeAR]

@kuraga even though I like the idea, I don't think it's actually possible to do that: functionality wise it's not identical to the Node.js version and due to the special handling in multiple cases it's important to update this module independent from Node.js versions (e.g., a shim turns out to be buggy).

[ljharb]

@BridgeAR thanks for the heads up; i'll take a look. @lukechilds what would be most helpful is if you could file a PR with failing test cases to https://github.com/ljharb/object.getownpropertydescriptors - i'm not sure how to reproduce the problem.

[lukechilds]

Edge is working 🎉

IE is being a massive pain, but nearly there.

Will continue this tomorrow and get IE support finished.

[lukechilds]

Wow, that was a real pain. I feel like about 50% of the code is now IE specific patches 🙃.

But it's working. ¯\_(ツ)_/¯

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

A couple of cleanliness/comment things I need to address but I think functionality-wise this should be done now.

[BridgeAR]

Supporting IE 11 is really bad in general. I hope usage will soon drop below 1% but right now it's still at 3%. I even know people who have to keep on using it (public schools where the teachers have no privilege to install or change software and nothing else exists on those machines).

I wonder if it makes sense to publish this as semver-major and then to publish another semver-major shortly afterwards that drops support for IE completely. That way it's easier to update and the code base would be significantly cleaner. People who would need to support IE would then use one version while others could use the newer one.

[kuraga]

I wonder if it makes sense to publish this as semver-major and then to publish another semver-major shortly afterwards that drops support for IE completely. That way it's easier to update and the code base would be significantly cleaner.

:+1:

[kuraga]

And let's move all shims/pollyfills to a separate file?

[kuraga]

Also I think before merging this PR we should collect all small patches (and close other PRs) and publish a minor version...

[lukechilds]

Ok, I've updated the readme, added some docs/explanations about the build system and how to contribute.

I've simplified the shims that had lots extra checks we don't really need.

I've also added comments at the top of any files with large chunks of tests commented out explaining the reasoning behind why they're disabled.

All test shims are exported from test/common/index.js and used in the tests like: common.includes, common.repeat etc.

In the actual source, the only place the shims are used is in internal/errors.js. So I've just left them inline in that file. It seemed more effort than it's worth to pull them out into a seperatae file to then only be pulled back into a single file.

It's based on this implementation from readable-stream which also has the shims inline: https://github.com/nodejs/readable-stream/blob/c7dee8c4d0e7315ab7ebf6ea3eb3c9669f00fb96/errors.js

I think this is fine but let me know if you think they should be pulled out.

As far as I can tell, this should be complete. So unless anyone has any more feedback I think this is ok to be merged/published.

[lukechilds]

@goto-bus-stop @BridgeAR Is there anything else you need from me for this PR or does it all look ok?

[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 (https://github.com/webpack/node-libs-browser/pull/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: https://github.com/msgpack/msgpack-javascript/pull/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:

https://github.com/browserify/commonjs-assert/blob/8386769eb15077c84b5e46f772d41bcb0b40651c/assert.js#L5-L37

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.

[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 assert@2.0.0 with these changes!