-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 unfriendly error; add ESM examples for root hook plugins; closes #4310 #4311
Conversation
- re-enable parallel-mode root hook plugin tests - refactor root hook plugin tests to avoid flake Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
Just so we understand what's happening with the error. This is the current behavior. Given: // foo.js
export const foo = 1; Running this $ mocha --require foo.js you get
With my change, you get instead
which is the root problem. (file must end in |
I considered trying to make this even friendlier--e.g., "invalid use of ESM, please see node docs". however, just because a file throws a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small test improvement suggestion, but this can land as is as well.
I was thinking, do we maybe want to refer to root hooks as being test "setup" functions as well? That way users that are searching for "setup" also endup at these docs. If you agree I can create a PR for it.
docs/index.md
Outdated
@@ -1316,7 +1335,9 @@ Available root hooks and their behavior: | |||
The root hook callbacks run in the usual context, so `this` is available: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root hook callbacks run in the usual context, so `this` is available: | |
The root hook callbacks run in the usual context, so `this` will refer to the current mocha context: |
this is a drawback to using ctrl-f for searching. we can’t attach keywords or metadata to a section... in general I’d like to avoid using multiple terminology for the same idea. “setup” is a “before each” hook in the tdd interface, for example, so it’d conflict |
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
4fe1ab5
to
66f16d1
Compare
This PR conflates two things, and I apologize.
I've updated the docs to show how to use ESM with root hook plugins.
I've also added a fix in the case that the
middleware
fails. It looks like yargs does not handle async middleware rejections properly--it fails to trap any error, and instead prints an unrelated problem. So, ifhandleRequires
orvalidatePlugin
fails, we need to trap that error ourself and print it, then force yargs to exit before thefail
handler can take over.Also contains refactors of
test/integration/options/require.spec.js
, which was driving me nuts due to the nondeterministic parallel tests. It was also confusing as hell anyway, so hopefully the result is a little easier to stomach (it's still a hack).