Sync with Node.js master

[BridgeAR]

Node.js 8 got support for Set and Map and some performance improvements and it would probably be nice to sync this library again.
I can open a PR if this is something that would be considered.

[graingert]

and strict mode

[lukechilds]

I've been asked to bring this module up to date by @jprichardson.

I think adding strict mode (#41) is the main difference which I've done (pending review).

However looking through the commit history for Node.js assert there are a lot of subtle changes that have been made in Node.js since this module was last updated: https://github.com/nodejs/node/commits/master/lib/assert.js

I think manually going through all those commits and trying to copy over new functionality would be quite time consuming and error prone. It would probably be quicker and less error prone to just copy the current source from Node.js master and make the required fixes to get this working in the browsers we're targeting.

We should also adopt the unit tests from Node.js master to guarantee consistent behaviour: https://github.com/nodejs/node/blob/master/test/parallel/test-assert.js

I have a few ideas that I think would make sense to do while bringing this up to date and will improve this project and make future maintenance and keeping in sync with Node.js much easier:

Copy source and tests directly from Node.js and fix to work in browsers

Much easier and quicker than trying to match the modern Node.js functionality with the current code base as they have diverged quite a lot.

Each file in this repo should include the a comment referencing the commit hash it was last updated from in Node.js.

Something like this at the top of the files:

// Currently in sync with Node.js lib/assert.js
// https://github.com/nodejs/node/commit/75463a9004a62d120e0590f86adb923f28c9cd13

// Currently in sync with Node.js test/parallel/test-assert.js
// https://github.com/nodejs/node/commit/1ed3c54ecbd72a33693e5954f86bcc9fd9b1cc09

That way keeping files in sync is as simple as viewing the commit history for that file in Node.js and diffing the tip of master against the commit in our comment. Then we'll have a simple diff of exactly what's changed since our last update.

These comments should be kept updated as we sync changes over.

Keep code as similar as possible to Node.js master

If there are blocks of code that simply can't be implemented in the browser version we should comment the block out and add a small comment explaining why it can't be implemented.

This would be better than just deleting the code block to keep the two codebases as similar as possible when comparing them. Could also help to prevent people wasting time trying to implement features they think are missing before realising there was a reason they weren't already implemented.

Transpile source code

Maybe a bit of a controversial one. I'm normally against introducing a build step unless absolutely necessary, however I feel there's a pretty huge benefit here. Node.js core can obviously always use the latest syntax. We can't which means the two codebases will end up looking very different even if they're functionally very similar. This is going to make it harder to compare the two code bases.

It would be much better if we can use almost identical syntax to Node.js and let a transpiler transform everything into syntax understood by our browser targets.

Obviously we'll still need to manually shim some APIs for old browsers but those don't affect the code structure too much.

Improve tests

Currently tests are quite out dated and are failing. I think the tests should just run on the latest Node.js for quick local testing and to catch any obvious bugs.

Then they should be run extensively against all browser targets to guarantee compatibility.

The current setup tests against many historic Node.js versions which complicates testing and doesn't really achieve much if browsers are the main target.

---

I understand these are some pretty radical changes but I think they will help a lot. As this pretty much changes everything in the repo (source/tests/build system) it's essentially a new project, it might even be worth doing this in a new repo (maybe browserify/assert?). Then deprecating browserify/commonjs-assert in favour of browserify/assert when it's completed and tested.

I think that would probably be neater than working in a branch and then merging essentailly a completely new project on top of this one. Although I'm fine with either if people think that's unnecessary.

Anyway, just throwing these ideas out there. I would like to crack on with this but just wanted some feedback before I started making any radical changes.

@goto-bus-stop What are your thoughts on this? Anyone else I should ping for feedback?

[BridgeAR]

@lukechilds

I think adding strict mode (#41) is the main difference

I disagree. The comparison algorithm is completely new, the error output is completely new, there is a new validation mode with assert.throws, assert.rejects was added, assert.ok prints the call site in case of failures and lots of other changes.
The introduction of the strict mode is the only thing I regret from all my changes to Node.js core. I originally planned to do something else but there where some concerns about that and strict mode was the alternative I came up with. AFAIK it is not used a lot and people could easily miss to add the strict somewhere and then have very loose comparisons even though they believe they use the strict version.

It would probably be quicker and less error prone to just copy the current source from Node.js master and make the required fixes to get this working in the browsers we're targeting.

Absolutely. I do not see a good alternative to that. A few things will be hard though. In Node.js core I use a few functions that are exclusively available in core.

It now also relies upon util.inspect which also uses some internal functionality such as: getOwnNonIndexProperties, getPromiseDetails, previewEntries. We also use a lot of util.types checks which can not all be polyfilled.

The first function is also used in the comparison algorithm and can be worked around by using Object.keys and Object.getOwnPropertySymbols. It's just slower that way.

It will be hard to port util.inspect. That became quite complex and without such port, it will be impossible to use the existing tests.

The code now also relies upon acorn and we'd have to port the Node.js core error functionality.

We should also adopt the unit tests from Node.js master to guarantee consistent behaviour

Yes, they are split into multiple files though, not only the single test file.

Transpile source code

I am all for it even though I am normally also against it. Otherwise even smaller things like destructuring etc would be quite some overhead.

it's essentially a new project, it might even be worth doing this in a new repo (maybe browserify/assert)

I already thought about forking and doing this as well, since I did not get any response so far and I am the main maintainer of Node.js core assert and util.

[goto-bus-stop]

I spent lots of hours already trying to port util.inspect to a wide variety of browser targets, but didn't get it over the finish line yet :(

I think for assert as a shim, the primary priority is to support all the functions, and do as much of the comparisons in an equivalent way. The error formatting and stuff is less important imo. Introducing acorn as a dep would definitely be a blocker :)

I don't have a strong opinion as to how the update should be done, we should aim for maximum browser compatibility though. Things that are shimmable in ES5 would be the limit for the most part. util and events for example work in IE9 because its ES5 support is ok, and can work in IE8 with es5-shim.

e; for continuity, IMO it would be best to continue to use this repository.

[goto-bus-stop]

fwiw Standard style recommends using assert.strict so it probably is seeing some use :)

[BridgeAR]

I think for assert as a shim, the primary priority is to support all the functions, and do as much of the comparisons in an equivalent way. The error formatting and stuff is less important imo. Introducing acorn as a dep would definitely be a blocker :)

The thing is: the Node.js core tests can only be used if all functionality is ported (or almost all). Otherwise it would be very hard to use them.

And even porting the comparison is already quite some work. I would do this by copying all files and creating some transpiling for some features to work with the newest browsers first. It can then later on be expanded to support older versions by transpiling more.

[lukechilds]

@BridgeAR

Thanks for your feedback, lots of valuable info there. Seems like this is a bigger job than I first anticipated. 😅

We should also adopt the unit tests from Node.js master to guarantee consistent behaviour

Yes, they are split into multiple files though, not only the single test file.

Do you mind linking me to the other test files? I'm not too familiar with the Node.js codebase.

I think for assert as a shim, the primary priority is to support all the functions, and do as much of the comparisons in an equivalent way. The error formatting and stuff is less important imo. Introducing acorn as a dep would definitely be a blocker :)

The thing is: the Node.js core tests can only be used if all functionality is ported (or almost all). Otherwise it would be very hard to use them.

I think it's good to include all the Node.js test files to keep the code bases consistent. But I think it's ok to comment out chunks of tests for behaviour that isn't crucial to support in a browser environment. (e.g Testing error message output)

I am the main maintainer of Node.js core assert and util.

This is good to know, is it ok if I ping you for advice if I get stuck with porting some parts?

@goto-bus-stop

I think for assert as a shim, the primary priority is to support all the functions, and do as much of the comparisons in an equivalent way. The error formatting and stuff is less important imo. Introducing acorn as a dep would definitely be a blocker :)

I agree, we just need something functionally equivalent or as good as can be achieved.

I don't have a strong opinion as to how the update should be done, we should aim for maximum browser compatibility though. Things that are shimmable in ES5 would be the limit for the most part.

Yeah, what I had in mind is similar to what @BridgeAR just suggested. I will probably just try to get everything working in Node.js 10 first. Just in pure JavaScript without using any core APIs.

Once that's working we can add a transpiler to target whichever browsers we need to support.

IMO it would be best to continue to use this repository.

Ok, I will create a new branch in this repo to work in. I can start on that tomorrow.

[BridgeAR]

@lukechilds

Do you mind linking me to the other test files? I'm not too familiar with the Node.js codebase.

Check test/parallel/test-assert* and test/pseudo-tty/test-assert*.

[...] I think it's ok to comment out chunks of tests for behaviour that isn't crucial to support in a browser environment. (e.g Testing error message output)

That is correct. There are just quite a few tests that verify the output.

This is good to know, is it ok if I ping you for advice if I get stuck with porting some parts?

Sure. I am also happy to help but I just did not want to get started without being sure that any of it would land and I never got any feedback on my request.

[lukechilds]

I understand. Seems like this module hasn't seen much activity since being moved over to the browserify org.

@goto-bus-stop I'm assuming if we can get this up to date with Node.js master there would be no objections to merging this? Looks like there will be some breaking changes in behaviour.

[goto-bus-stop]

Yes breaking changes from node should also land here! Thanks for picking this up

…

On April 10, 2019 5:55:05 PM GMT+02:00, Luke Childs ***@***.***> wrote: I understand. Seems like this module hasn't seen much activity since being moved over to the browserify org. @goto-bus-stop I'm assuming if we can get this up to date with Node.js master there would be no objections to merging this? Looks like there will be some breaking changes in behaviour. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #32 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.