Skip to content
New issue

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

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

Already on GitHub? # to your account

Eliminate need for Node assert in browser tests #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acorncom
Copy link

Howdy, I'm upgrading a client app from Webpack 4 -> 5 (yes, I know it's old). As part of that, we hit the below wrinkle with timezone-mock:

ERROR in ./node_modules/.pnpm/timezone-mock@1.3.1/node_modules/timezone-mock/index.js 1:24-41
Module not found: Error: Can't resolve 'assert' in '/home/circleci/project/node_modules/.pnpm/timezone-mock@1.3.1/node_modules/timezone-mock'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "assert": require.resolve("assert/") }'
	- install 'assert'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "assert": false }
resolve 'assert' in '/home/circleci/project/node_modules/.pnpm/timezone-mock@1.3.1/node_modules/timezone-mock'
  Parsed request is a module
  using description file: /home/circleci/project/node_modules/.pnpm/timezone-mock@1.3.1/node_modules/timezone-mock/package.json (relative path: .)

It turns out the polyfill for assert is relatively heavy in a browser context and assert usage here was pretty minimal. So thought I'd send this over as a possible fix. I can add better comments or a test to check in a browser context if desired, but thought I'd check to see if you were at all interested in the change.

Thanks for your work on this!

Copy link
Owner

@Jimbly Jimbly left a comment

Choose a reason for hiding this comment

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

Generally sounds good to me!

@@ -292,7 +299,7 @@ function unregister(glob) {
}
}
if (glob.Date === MockDate) {
assert(_Date);
assert.ok(_Date, 'need to pass a date');
Copy link
Owner

Choose a reason for hiding this comment

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

More accurate message is probably "unregister() called but missing original Date constructor".

@@ -1,6 +1,13 @@
'use strict';

var assert = require('assert');
var assert = {
ok(cond, msg) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could probably just be function assert(cond, msg) { as we only use the default export of assert here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants