Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support canonical module #5040

Merged
merged 5 commits into from
Jul 20, 2024
Merged

Conversation

JacobLey
Copy link
Contributor

@JacobLey JacobLey commented Dec 4, 2023

ORIGINAL PR: #4888
Closed out of staleness, but now trying to re-open under new maintenance

Description of the Change

Handle canonicalizing Module, prevent implicit stringification failures
Alternate Designs

Have canonicalType return 'object' for modules (not chosen as that could be breaking, not sure...)
Have 'module' fallthrough to 'object' with no custom logic (not chosen so we can indicate different between "object" and "module")

Why should this be in core?

Current module canonicalization is broken, see #4887
Benefits

Tests can properly handle errors with a Module inside
Possible Drawbacks

Small chance there is some internal/custom handling expecting canonicalization of modules to fail... Seems unlikely?
Applicable issues

Fixes #4887

Bug fix, patch release

@coveralls
Copy link

coveralls commented Dec 4, 2023

Coverage Status

coverage: 94.364% (+0.03%) from 94.337%
when pulling b4b7066 on JacobLey:canonicalModule
into eca4fec on mochajs:master.

@@ -288,6 +290,27 @@ describe('lib/utils', function () {
].join('\n');
expect(stringify(expected), 'to be', actual);
});

it('should represent modules', async function () {
if (process.browser) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The esm-utils and node:path modules both come through as undefined in browser (which is kinda expected).

In theory though, it probably isn't necessary to use esm-utils at all, and the path logic we are using is just a very basic join, which we can probably safely default to __dirname + '/fixtures/module.mjs' and load via a dynamic import import() (Maybe keep the path module around for node+windows support)

However, it appears the current version of rollup has issues parsing that. It appears rollup is ~2 major versions out of date, so I wouldn't be surprised if it just isn't equipped to handle "modern" js syntax.

I tried bumping rollup version and it was not straightforward. I'm sure someone with more rollup experience can easily knock it out, but it felt out of scope for what I was trying to achieve with this fix.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft March 4, 2024 17:44
@JoshuaKGoldberg
Copy link
Member

Converting to draft pending triage of #4887. cc @JacobLey

@JoshuaKGoldberg JoshuaKGoldberg added the status: blocked Waiting for something else to be resolved label Mar 4, 2024
@JacobLey
Copy link
Contributor Author

Updated original ticket with better reproduction: #4887

Still open to alternative approaches to handle the "this should only run as part of serverside tests", but that is more on the testing front rather than the implementation.

lib/utils.js Outdated
@@ -399,6 +399,12 @@ exports.canonicalize = function canonicalize(value, stack, typeHint) {
break;
}
/* falls through */
case 'module':
Copy link
Contributor Author

@JacobLey JacobLey Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a bit more manual testing (when trying to reproduce) I think this issue may fundamentally occur anytime you are dealing with null-prototyped instances...

Which is an overall rare occurrence, but maybe this isn't the right approach?

e.g.

const foo = Object.create(null, { 
    [Symbol.toStringTag]: { value: 'Foo' }, 
     bing: { get: () => 'bong' } 
});

Object.prototype.toString.call(x); // '[object Foo]'

foo + ''; // Uncaught TypeError: Cannot convert object to primitive value

So while this solution works for modules, I don't think it actually solves the underlying issue

});
const expected = [
'{',
' "[Symbol.toStringTag]": "Foo"',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to either:

  1. Always expose the toStringTag if present
  2. Never expose the toStringTag

Then we could just return object instead of null-prototype and the logic would work.

Only really need a special case if we are conditionally exposing it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference either way. What's working now fixes the issue and is nicely informative on my end.

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue and removed status: blocked Waiting for something else to be resolved labels Jul 2, 2024
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review July 2, 2024 15:22
@JoshuaKGoldberg JoshuaKGoldberg added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Jul 2, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks - and with great tests! Confirmed that linking locally fixes the issue. ✅

Will wait a bit for another reviewer just to be safe.

@JoshuaKGoldberg JoshuaKGoldberg removed the status: in triage a maintainer should (re-)triage (review) this issue label Jul 2, 2024
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally approve 👍

lib/utils.js Outdated
Comment on lines 150 to 154
} else if (
typeof value === 'object' &&
// eslint-disable-next-line no-prototype-builtins
!Object.prototype.isPrototypeOf(value)
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably do this instead? And avoid having to opt out of the eslint rules?

Suggested change
} else if (
typeof value === 'object' &&
// eslint-disable-next-line no-prototype-builtins
!Object.prototype.isPrototypeOf(value)
) {
} else if (Object.getPrototypeOf(value) === null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that is cleaner, pushed a small update

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀

@JoshuaKGoldberg JoshuaKGoldberg merged commit 579e047 into mochajs:main Jul 20, 2024
24 checks passed
@JoshuaKGoldberg
Copy link
Member

Released in [email protected]. Thanks again @JacobLey! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Modules in errors cause unhandledRejection, tests quit early
4 participants