browserify / commonjs-assert

Node.js's require('assert') for all engines
MIT License
295 stars 58 forks source link

deepEqual(true, [ 1 ]) #26

Closed mk-pmb closed 7 years ago

mk-pmb commented 7 years ago

Hi! I'm developing an assert lib and it disagrees with this one:

~/commonjs-assert$ git-log-concise -n 1
Mon 2016-06-27 02:58    f8975ca Add deepStrictEqual to the README (#23)

~/commonjs-assert$ nodejs -e 'require("./assert.js").deepEqual(true, [ 1 ]);'
[…]
AssertionError: true deepEqual [ 1 ]

I'd prefer my lib to also throw an error on this, but I can't find permission for that in the CJS Unit Testing 1.0 spec. According to my lib, rule 7.3 applies, because one of the values is not an object, so they don't "both pass" the typeof object test. As said, I prefer your module's behavior, so what reading of the spec did you come up with to legitimate throwing?

Node.js also throws, but to me it seems it's unintentional there.

calvinmetcalf commented 7 years ago

this is the same as the node one so we did whatever they did

mk-pmb commented 7 years ago

I see, I probably didn't read the Readme proper enought the first time.

The API is derived from the commonjs 1.0

Well, "derived" is a weak term indeed, so do you even claim that this module conforms to the CJS spec? If not, the module name and line 45 of assert.js would be quite confusing.

calvinmetcalf commented 7 years ago

This is literally the node code pulled out and slightly modified to work in a browser

On Wed, Oct 12, 2016, 3:29 PM M.K. notifications@github.com wrote:

I see, I probably didn't read the Readme proper enought the first time.

The API is derived from the commonjs 1.0

Well, derived is a weak term indeed, so do you even claim that this module conforms to the CJS spec? If not, the module name and line 45 of assert.js https://github.com/defunctzombie/commonjs-assert/blob/master/assert.js#L45 would be quite confusing.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/defunctzombie/commonjs-assert/issues/26#issuecomment-253313723, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4nw5iX2Q90KGQ9t7iib4JFAomDphbks5qzTUAgaJpZM4KVF-h .

mk-pmb commented 7 years ago

Yeah, that part I understood, thus my question is now about mission and intent. Only when I understand the goals of this module, can I decide which of two possible features I'd like to request. Do I understand correctly that

modified to work in a browser

was the only intent for this package, and it is not commonjs-assert's objective to be an assert module according to the CommonJS Unit Testing spec?

Edit: Updated the title to better represent the current state of the issue. Edit 2: Actually, I can split that one even now, since I'd want the answer in the readme anyway.

ScottFreeCode commented 7 years ago

For what it's worth, I'd rather like to see the relationship of this library to the Node internal module made more explicit in the readme and leave the matter of intention and spec compliance to the Node documentation (but, perhaps link that documentation here).

mk-pmb commented 7 years ago

Issue became irrelevant because we clarified that this assert doesn't aim to conform to the CommonJS spec.