-
-
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
alias lookupFiles, loadRc, loadPkgRc and loadOptions to lib/cli module; fixes #4398 #4419
Conversation
fixes #4398 - fixes API documentation for all of these (and `module:lib/cli.main`) via JSDoc aliasing - aliases `lib/cli/cli.js` to `module:lib/cli`; `module:lib/cli/cli` is no longer a thing - the `lib/cli/options.js` and `lib/cli/lookup-files.js` _modules_ (not necessarily their _contents_) are now private example usage: ```js const {loadRc, loadPkgRc, loadOptions, lookupFiles} = require('mocha/lib/cli'); ```
lookupFiles() broke (moved) in version v8.1.0, this returns it and issues a soft deprecation warning. in the browser, this function should not be called, but if it is, an `ERR_MOCHA_UNSUPPORTED` `Error` is thrown
@mochajs/core I know y'all had some thoughts about #4398, so would appreciate feedback here. I think it's important to fix the breaking change and issue a deprecation notice, but I won't protest if you'd like me to revert my other API-surface changes here. |
I moved |
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.
Good to add this API back. 👍
yes, it does not appear in the bundle. |
Will fix #4422 here as well. |
@Munter to be clear, I don't disagree that file-touching stuff should not be in @mochajs/core we can move |
please note that this now adds webpack to our development dependencies 😦 . probably better to do declare it explicitly than to just |
515d4b9
to
72346e3
Compare
A future major to move these out of the browser graph would be great. I have a few ideas for attempting a cleaner cut that I can start a draft of soon |
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.
LGTM
As per request for comment here: #4398 (comment) In terms of whether we need globbing to be part of Mocha, the answer is no. It's not difficult to do globbing ourselves. However, what we are trying to do when calling Thus, my argument is one for consistency. As boneskull stated, something like globbing being part of mocha is "of dubious necessity" as third party globbing tools could be used. If that's the case, though, I would expect to also need to do globbing beforehand if I was calling mocha from CLI or in a bash script. Basically, I advocate that one shouldn't have to relearn a new approach to calling a tool like mocha for every method of interaction it supports. It should be basically the same overall approach. Regardless, thank you for your time and thoughtful consideration in maintaining this library. |
module:lib/cli.main
) via JSDoc aliasinglib/cli/cli.js
tomodule:lib/cli
;module:lib/cli/cli
is no longer a thinglib/cli/options.js
andlib/cli/lookup-files.js
modules (not necessarily their contents) are now privateexample usage:
UPDATE
lookupFiles
tolib/utils
to un-break the breaking change introduced in v8.1.0.lookupFiles
inlib/utils
will print a deprecation notice directing the consumer to the new pathlookupFiles
inlib/utils
in a browser (since it's accessible) will throw an exception.ANOTHER UPDATE
require.cache
-touching code out oflib/mocha.js
and intolib/nodejs/file-unloader.js
; excludes this file from the bundle