Move the assert module to userland

[papandreou]

I feel that maintaining the assert module as part of node has been an unnecessary distraction for the project for a long time. If you search for assert across issues and PRs, 2360 results turn up. Now major features are being added, such as diffs and colored error messages, so it seems like it's only getting worse.

At the same time the module is classified as "stable" and hasn't been able to correct all the weirdness of the original CommonJS spec, leading to surprises when users don't use the "strict" and "deep" versions of the methods.

Liberating the module from core would allow it to evolve, and hopefully new users would stop picking it up "by default" and being surprised by its quirks.

[BridgeAR]

I am surprised that adding new features is considered "bad". I would have expected it to be good that this module is finally getting in nice shape. And I personally have been involved a lot in further optimizing assert but I do not see that this has distracted Node.js.

There are some quirks that can not easily be removed due to historical reasons but that is what the new strict mode is about. That way it is safe to use any function without worrying about using the "correct" function.

assert was part of Node.js for a long time and it is heavily used and moving it out is not really viable as far as I see it.

[Munter]

Expanding the scope of assert to include diffs and colored error messages is a Pandora's box of style fixes and introduces a huge amount of subtle bugs that need to be fixed. Fixing those in node core on a slow release cycle seems like a quite painful flow.

Looking through the colored diff that just went in, this also means node core now includes hard coded knowledge of several specific terminals and CI environments. This is a quite rapidly changing landscape, so this code will get out of date. Also add a maintainer of environments that have not been blessed by node core I question why my specific thing is not deemed worthy of color.

Are these really discussions that belong in what keeps being referred to as a small core?

[BridgeAR]

@Munter color support and if assert should be in Node.js are two separate concerns. The first is something that is absolutely not limited to assert and opens up the possibility for more features.
Having a small core is also a topic on it's own because it is a general thing and not about having a specific module. Not to mention that there are different opinions about having a small core or not as well.

Speaking about bugs before there is an issue is somewhat surprising to me. If Node.js is good in one thing though, it is fixing bugs. That is done all the time and does not have a slow release cycle. That is done almost once a week. What is slow is breaking code.

The current color detection is something that will likely need some further work but having that in Node.js is important, otherwise using colors is not really an option. I am traveling this week though and am fairly busy, so I did not get back to it yet.

[alexjeffburke]

@BridgeAR I don't think anybody was suggesting new features in core are bad, but we are asking for the rationale behind this being included in core.

Of course colour detection would be required if colours are desired in core, but the underlying question is exactly whether this is this really territory for core or is rather much better solved in userland? At least from my perspective it has been encouraged to do user facing libraries in the ecosystem for a long time and there are some amazing solutions that already exist there.

Also, I'd really like to see a specific response to the point raised by @Munter about hard coding terminal knowledge in core - how will this be handled? If this went in, would there be flags or environment variables added to force coloured output for example in CI (this is well known to require a helping hand - see --color options to all the test runners) or if my system happens not to support it? There things would need to be addressed, though I stand by my reservations.

Again, any and all work done by folks on core is very much appreciated by those of us using the platform. In this case however there are well documented long standing issues with the assert API. I don't think further encouraging it's use is the right thing and, as someone who has written a library that aimed for bug-for-bug compatibility with assert to provide users an upgrade path, I would be pretty worried about introducing changes to the module.

[benjamingr]

Here is the thing:

• Assert is in core right now
• It is pretty well maintained
• For non-core usage, a ton of userland alternatives exist.
• Taking it out of core right now would require non-trivial work
• Maintainers don't see maintaining it as a burden - or at least no core collaborators have expressed frustration over having to maintain assert.

So I am -1 for removing assert, mostly because it will undoubtedly "break userland" in some subtle way, it would require more work than it would to keep things as they are - and people don't really feel this is a big maintenance burden.

If the discussion was about introducing assert I believe the barrier would have been different.

[SimenB]

(Tangentially related, happy to open up a separate issue if you want.)

While I don't have any opinions on whether assert should be outside of Node core or not; as a maintainer of Jest, changing the output of assert makes it harder to integrate properly. We've had requests to Just Work ™️ with assert because it's in core, which is fine (and useful), but if we're going to have to clean the default output things might get unwieldy (already pretty gnarly: https://github.com/facebook/jest/blob/358764dcc784028e5e2d231a0e40e02688fc532b/packages/jest-jasmine2/src/assert_support.js). So I'd appreciate if the plain error message, without diffs or colors, be added as a separate property to the thrown error.

EDIT: Or even better: having structured data in general on the error

[alexjeffburke]

@SimenB that's a really good point! I wrote a drop in replacement for assert (https://github.com/alexjeffburke/assert-the-unexpected) that emulates the API but runs assertions via the Unexpected assertion framework. Part of this emulation was to make sure thrown errors were still of type AssertionError and the messages matched in case anybody was checking the contents of the .message property and I'm not really what effect the colouring code will have (especially if it's on by default).

Finally, in the spirit of trying to share knowledge - there are some pitfalls with extending messages and at least a large number of mocha releases need "message" to equal the start of "stack" or things get printed twice.

[benjamingr]

@SimenB - hey, if you're up for it we're doing a session for user feedback and tooling folk - you're more than welcome to provide this feedback directly (it's welcome and will likely have an impact) nodejs/user-feedback#37

So I'd appreciate if the plain error message, without diffs or colors, be added as a separate property to the thrown error.

That can also be a feature request or a configuration set with an environment variable (CC @BridgeAR @addaleax ), there is no need to go on tangents you're welcome to open an issue and request it as a feature request.

[cjihrig]

Moving assert to userland isn't possible, as core uses it for its tests (not to mention the ecosystem breakage that would follow). For that reason, I'm going to close this issue, although I definitely sympathize with the motivation behind opening this issue.

[SimenB]

@benjamingr sounds good! I'll create an issue after Easter

[papandreou]

I'm a bit disappointed that this issue is closed before the discussion even started.

First off, the fact that core uses assert for its tests is not a good argument for also maintaining it as part of core, and especially not for exposing it to userland.

I agree that it would be unrealistic for core to stop exposing the module at this point, even with a long, well thought out deprecation strategy. Still, I hoped that we could have a strategic discussion about this and maybe even agree that:

1. Exposing assert was a mistake in the first place (check)
2. The more advanced assert becomes, the more hopeless it is to develop and version it along with node.js itself because of backwards compatibility
3. For the above reasons we should discourage new users from using assert

The new features are probably fine and well implemented, but I still think it's bad judgement to land them, thus further encouraging the use of assert. Please just keep that module as simple as possible, seeing that we cannot get rid of it. I maintain that it's a needless distraction for the project.

[ljharb]

Whether something is a distraction or not seems like something only the maintainers can judge.

[papandreou]

@ljharb, as a contributor, observer and user I believe I'm entitled to an opinion?

[jasnell]

I'm quite sympathetic to this idea and have often considered proposing that the top level assert module be deprecated in core with any additions/changes necessary for core's own use and testing be moved to internal or test support code (via test/common.js). While the module is ok for what it does, there is little value in it as a userland asssertion API and very little value in maintaining it as a core API (despite it being relatively simple and painless to maintain).

[benjamingr]

@papandreou

as a contributor, observer and user I believe I'm entitled to an opinion?

Your participation and contribution is always welcome, it doesn't look like any of your prior commits to Node.js are in the assert module or in its area.

Everyone responding not from core (@papandreou , @Munter, @alexjeffburke)* don't have a whole lot of prior participation in assert and are all affiliated with the same company ( https://github.com/One-com - that does seem to be related to consoles with colors if I'm reading this correctly).

I appreciate your contributions to Node and the participation here but I'm not sure it's fair to say the issue was closed without discussion. Multiple TSC members (James and Colin) and multiple core collaborators me and Ruben) have responded on the removal of assert from core.

Honestly we just don't see the motivation for doing so much work and causing the ecosystem so much breakage for something we don't see maintaining as a burden.

• Except SimenB from Jest - who did not voice an opinion for removing assert from core anyway.

[alexjeffburke]

@benjamingr I don't think our affiliation is at all relevant and for the record I'm the only person on this thread that works at One. The reason for our commentary here is all of us have worked for years building out and contributing to an assertion framework and a whole host of plugins - what you see on GitHub, spread across our accounts, is the work that it has taken to achieve this of which an enormous amount if not the majority was done in spare personal time. Those experiences, particularly the complexity of doing this, is the synthesis for this commentary.

Removal of assert might not be feasible, but I do think it's more than fair for us to ask whether giving it additional features will encourage it's use and whether that would be a good thing. I'd also like to say that we're glad to see some of the ideas about helpful diffs that assist users catch on, but we're also cautioning that it may come with a significant as yet unseen maintenance burden.

You mentioned we never contributed to assert, but for a long time the module was locked and it was made clear that contributions were to be made in userland - advice that we took to heart. If there has been a policy change here it's something I've not seen communicated and I'd like to understand it. We can only do that through discussions like this and I appreciate yourself, other maintainers and those on the TSC taking time to engage in dialogue with us around this.

[papandreou]

@benjamingr, I was mostly responding to what seemed like an unnecessary rank pull, but thanks for taking it literally and unfolding my life story :)

I don't like the notion that it requires some kind of prequalification to be heard, but I'll bite. The reason why I even have an opinion is that I've spent many hours thinking about assertion semantics and rendering of diffs while contributing to the assertion library Unexpected.js, mocha and others.

Let me assure you that I'm not worried about having another competitor. I'm just painfully aware how big a task it is to get the semantics right, and the amount of bikeshedding around rendering of array/object/type-aware diffs. I don't think it makes sense for core to take on those tasks. I'd be happy to elaborate on the things that we have struggled the most with in this area, if you'd like to pursue it further.

[benjamingr]

@papandreou @alexjeffburke if you would like to get more involved with how assert works in core, as well as have critical feedback on newish features being worked on (like Ruben's on diffs) that would be fantastic and well appreciated.

More general, your feedback and contributions on the assert built in module, as well as more formal community feedback would be appreciated.

Raising pain points and complexity involved in those tasks would be extra important as it would help us figure out issues before we even run into them. We mostly enhance assert to make our lives better so that our internal tests run better.

One thing we can't do though is to remove assert entirely since it'll cause major ecosystem breakage. We have had a very painful experience with removing things that were much much smaller. Node.js takes keeping our users happy very seriously.

One thing we could consider is adding a note to the docs indicating this isn't a high level module (but I guess that's true for everything else in core).

I understand this might come off as frustrating, you combined likely have more assertion library experience and diff experience than us - but given our constraints (not breaking userland) I think it would be fantastic if you helped bring that experience to the table by getting more involved. If you're not sure where to start or what contributions would be well received then that's something I can definitely help with.

[Munter]

@benjamingr

We mostly enhance assert to make our lives better so that our internal tests run better.

I'm a bit confused here. Is there a policy against taking user land module into the node test suite to improve the developer experience? People, both in this thread, but also on other assertion libraries and diffing tools, have literally spent years thinking about this one problem and iterated to improve the developer experience for a long time. It seems wasteful to take a not-invented-here approach

[benjamingr]

@Munter if there will be compelling reasons to use a third-party module for our internal tests then that's something I think we can and should consider.

That's not the same thing as removing assert from core, the amount of breakage would be huge. There are things that have been deprecated in Node for 3 years now (like fs.exists) that we have no intention of removing. The thought of actually removing an API that is 2 - Stable and was 3 - Frozen makes me shudder a little to be honest.

[joyeecheung]

As a user of assert, I do appreciate the new features as they make the results much easier to read by default, but as a maintainer I'll have to say I share the sentiment as the OP, especially when user-land eye candies like power-assert exists (sorry that I have not paid enough attention while the features were being added and didn't speak up before, had I know they were coming I would probably express -1 on the PRs).

Removing assert does not seem to be practical to me, but the fact that assert is used internally means we need to pay more attention to its size. In case you have not noticed:

./node -p 'process.moduleLoadList'
[ 'Binding contextify',
  'NativeModule events',
  'NativeModule internal/async_hooks',
  'NativeModule internal/errors',
  'Binding uv',
  'Binding buffer',
  'Binding async_wrap',
  'NativeModule internal/v8',
  'Binding v8',
  'Binding config',
  'Binding icu',
  'NativeModule util',
  'NativeModule internal/encoding',
  'NativeModule internal/util',
  'Binding constants',
  'Binding util',
  'NativeModule internal/util/types',
  'Internal Binding types',
  'NativeModule buffer',
  'NativeModule internal/buffer',
  'NativeModule internal/util/comparisons',
  'NativeModule internal/process',
  'NativeModule assert',
  'NativeModule fs',
  'NativeModule path',
  'NativeModule internal/constants',
  'Binding fs',
  'NativeModule stream',
  'NativeModule internal/streams/legacy',
  'NativeModule _stream_readable',
  'NativeModule internal/streams/buffer_list',
  'NativeModule internal/streams/destroy',
  'NativeModule internal/streams/state',
  'NativeModule internal/streams/async_iterator',
  'NativeModule _stream_writable',
  'NativeModule _stream_duplex',
  'NativeModule internal/streams/duplex_base',
  'NativeModule _stream_transform',
  'NativeModule _stream_passthrough',
  'Binding fs_event_wrap',
  'NativeModule internal/fs',
  'NativeModule internal/url',
  'NativeModule internal/querystring',
  'NativeModule querystring',
  'Binding url',
  'NativeModule internal/deps/acorn/dist/acorn',
  'NativeModule os',
  'NativeModule internal/os',
  'Binding os',
  'NativeModule internal/process/warning',
  'NativeModule internal/process/next_tick',
  'NativeModule internal/process/promises',
  'NativeModule internal/process/stdio',
  'Binding performance',
  'NativeModule perf_hooks',
  'NativeModule async_hooks',
  'NativeModule internal/linkedlist',
  'NativeModule internal/trace_events_async_hooks',
  'Binding trace_events',
  'NativeModule internal/inspector_async_hook',
  'Binding inspector',
  'NativeModule timers',
  'Binding timer_wrap',
  'NativeModule internal/timers',
  'NativeModule internal/modules/cjs/loader',
  'NativeModule vm',
  'NativeModule internal/modules/cjs/helpers',
  'NativeModule internal/process/esm_loader',
  'Internal Binding module_wrap',
  'NativeModule internal/modules/esm/loader',
  'NativeModule internal/modules/esm/module_map',
  'NativeModule internal/modules/esm/module_job',
  'NativeModule internal/safe_globals',
  'NativeModule internal/modules/esm/default_resolve',
  'NativeModule url',
  'NativeModule internal/modules/esm/create_dynamic_module',
  'NativeModule internal/modules/esm/translators',
  'NativeModule console',
  'Binding tty_wrap',
  'NativeModule tty',
  'NativeModule net',
  'NativeModule internal/net',
  'Binding cares_wrap',
  'Binding stream_wrap',
  'Binding tcp_wrap',
  'Binding pipe_wrap',
  'NativeModule internal/stream_base_commons',
  'NativeModule dns',
  'NativeModule readline',
  'NativeModule string_decoder',
  'Internal Binding string_decoder',
  'NativeModule internal/readline',
  'Binding signal_wrap' ]

The appearance of acron was how I got to learn about the new features before I actually noticed the difference in the tests. There are replacements for assert internally now (the CHECK macros) but I am somewhat skeptical about replacing the internal asserts in bulk because those macros abort instead of throwing.