assert: move AssertionError to prevent lazy loading

[BridgeAR]

This prevents lazy loading util.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

assert

[jasnell]

-1 ... I moved this in with the eventual goal of tying it into the internal/errors mechanism better later on. This move was recent and I'd prefer not to move it out.

[refack]

IMHO you could remove the use of assert in errors, AFAICT it's only used for asserting the proper usage of the methods that are internal anyway.

[gibfahn]

-1 ... I moved this in with the eventual goal of tying it into the internal/errors mechanism better later on. This move was recent and I'd prefer not to move it out.

Is there a reason to keep it in now? I don't really follow the logic behind moving things in preparation for other things. Presumably it can be moved out in the same PR that "ties it better into the internal/errors mechanism".

@BridgeAR Having said that, I'm also not sure what the benefit of this PR is, I assume it's for performance reasons, if so some benchmarks would be good.

[BridgeAR]

I think removing lazy loading in general is a good thing if possible (and if it does not have a negative impact on the common case) and the AssertionError belongs to the assert module for me and not in the internal/errors. I personally also do not really follow the logic to move things in preparation. @jasnell should they inherit from NodeError? In that case I could just open a PR to do that instead of moving them to the assert module again?

As the performance difference is neglectable after the change to a simple if, I do not have a strong opinion about it and I can close this if you'd like me to.

[refack]

@BridgeAR Having said that, I'm also not sure what the benefit of this PR is, I assume it's for performance reasons, if so some benchmarks would be good.

Lazy loading is a hack to solve circular dependencies. If we can remove the circular dependency, that's best. AFAICT there's no need to use assert in internal/errors.js (used just in 2 LOCs anyway). internal/errors.js is an internal module so all assertions should be done in /test/ not in runtime.

[BridgeAR]

Ping @jasnell

[jasnell]

I'd be fine with eliminating the asserts in internal/errors

[BridgeAR]

@jasnell #15002 is actually resolving a different circular dependency then this one. This PR was not about the assertions in internal/errors but about the util circular dependency in the AssertionError and about the reasons listed in #14350 (comment).

As I resolved a different circular dependency in assert that removes util from being loaded there this would result in the same situation as before. Therefore I am closing this.