-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat(create-jest): Add npm init
/ yarn create
initialiser
#14453
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/create-jest/package.json
Outdated
"./bin/create-jest": "./bin/create-jest.js" | ||
}, | ||
"dependencies": { | ||
"jest-cli": "workspace:^" |
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.
This means that the whole Jest with all its dependencies will be pulled? Or? I would expect the create-
package to be a lightweight script. Did I miss something?
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.
Perhaps it makes sense to move the whole logic to the new package and deprecate the --init
flag in Jest 30?
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.
This means that the whole Jest with all its dependencies will be pulled? Or? I would expect the create- package to be a lightweight script. Did I miss something?
I was thinking about standardised global wrapper for the initialisation (adapter for npx jest --init
). For this purpose whole jest-cli
is not necessary indeed.
Perhaps it makes sense to move the whole logic to the new package and deprecate the --init flag in Jest 30?
Sounds fine, or maybe we can reverse the dependencies and use create-jest
inside jest-cli
to keep the API the same
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.
In this case create-jest
would get shipped with jest
. I was thinking a new package like @jest/init
could be introduced, but that requires major release as well. Other question, is there a need to have the --init
command if create-jest
gets introduced? By the way, moving the whole --init
logic to create-jest
would make jest
leaner (e.g. only create-jest
would ship prompts
as dependency, if I didn’t miss something).
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.
One more idea, I think for now it would be fine to copy the code to create-jest
and in jest-cli
it could be marked with // TODO Remove in Jest 30
comment.
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.
That's right. It can reused, I agree.
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.
To summarize: in addition to this
- This PR will move the entirety of the
--init
command implementation into this new packagejest-cli
will then have a dependency oncreate-jest
, and delegate to it whenjest --init
is called- A separate PR will be prepared for v30 (I'll add it to the milestone so we don't forget) which deprecates
--init
and removes the dependency
we are also adding [rootPath]
to JS and CLI API, right?
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.
we are also adding
[rootPath]
to JS and CLI API, right?
How do you feel? I find it useful, but can be implemented later.
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.
Yeah, I think that's fine. --init
will default to process.cwd()
, but I do think it makes sense as part of the API
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.
How do you feel? I find it useful, but can be implemented later.
I also find it useful, makes sense to do it here I think, it's not that hard
@@ -17,7 +17,7 @@ export type PromptsResults = { | |||
useTypescript: boolean; | |||
clearMocks: boolean; | |||
coverage: boolean; | |||
coverageProvider: boolean; | |||
environment: boolean; | |||
coverageProvider: string; |
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.
Sorry for the extra diff, I had to fix tslint
errors in this package. jest-cli
code was ignored, but new package name ends with jest
and it matches the condition in tslint
script 😄
I decided to do a good thing and fix the errors instead of excluding the package from the script
I'll prepare separate PR with |
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.
Thanks! Left few quick notes.
Could you add create-jest
to this list, please:
Line 114 in f2c78d0
"typecheck:tests": "tsc -b packages/{babel-jest,babel-plugin-jest-hoist,diff-sequences,expect,expect-utils,jest-circus,jest-cli,jest-config,jest-console,jest-snapshot,jest-util,jest-validate,jest-watcher,jest-worker,pretty-format}/**/__tests__", |
jest-cli
is there already, test files did not change, so the typecheck should pass.
import * as fs from 'graceful-fs'; | ||
import prompts = require('prompts'); | ||
import yargs = require('yargs'); |
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.
Just thinking out loud: it feels too much to ship yargs
with this package. I think process.argv[2]
would be to do the job. The value will be always string | undefined
.
Currently this package is 15.5 kB (unpacked). yargs
takes 292 kB (unpacked, without dependencies). That’s what npm gave to me.
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.
Sounds fair, I think keeping this package light is more important than having universal parsing for arguments and --help
interface
@@ -58,7 +88,7 @@ export default async function init( | |||
fs.existsSync(path.join(rootDir, getConfigFilename(ext))), | |||
); | |||
|
|||
if (hasJestProperty || existingJestConfigExt) { | |||
if (hasJestProperty || Boolean(existingJestConfigExt)) { |
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.
if (hasJestProperty || Boolean(existingJestConfigExt)) { | |
if (hasJestProperty || existingJestConfigExt != null) { |
? getConfigFilename(existingJestConfigExt) | ||
: path.join(rootDir, getConfigFilename(jestConfigFileExt)); | ||
const jestConfigPath = | ||
typeof existingJestConfigExt !== 'undefined' |
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.
typeof existingJestConfigExt !== 'undefined' | |
existingJestConfigExt != null |
): Promise<void> { | ||
rootDir = tryRealpath(rootDir); | ||
// prerequisite checks | ||
const projectPackageJsonPath: string = path.join(rootDir, PACKAGE_JSON); |
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.
Interesting why ESLint did not catch this:
const projectPackageJsonPath: string = path.join(rootDir, PACKAGE_JSON); | |
const projectPackageJsonPath = path.join(rootDir, PACKAGE_JSON); |
import * as fs from 'graceful-fs'; | ||
import prompts = require('prompts'); | ||
import yargs = require('yargs'); | ||
import {getVersion} from '@jest/core'; |
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.
Importing @jest/core
will ship almost the whole Jest with create-jest
. Not sure if that is worth.
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
const importLocal = require('import-local'); |
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.
Hm.. import-local
is not included in dependencies
list?
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.
Also not so sure if import-local
is needed in this case. It does not make sense installing create-jest
locally. To be honest, I don’t understand what import-local
does ;D So it can be I missed something.
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.
I'm ok with removing that, local installation indeed seems strange in this case
try { | ||
const argv = await yargs(process.argv.slice(2)) | ||
.usage('Usage: $0 [rootDir]') | ||
.version(getVersion()) |
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.
If it is decided to keep yargs
, this should be create-jest
version. It will be used by create-jest --version
, or?
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.
Removed yargs
to keep package lighter
Co-authored-by: Tom Mrazauskas <[email protected]>
…/jest into feat/create-jest-package
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.
Thanks! Looks alright for me.
Not very happy to see runCLI()
function being exported, but I understand that there are some limitation since --init
has to reuse the logic. This can be cleanup up in the future.
Just a detail, there is no mention of Hm.. Hard to say where it belongs. Does it make sense to suggest Perhaps for now |
"Getting Started" makes the most sense to me |
@mrazauskas I also don't like it, actually we don't need it in So I can remove the export, but in |
packages/create-jest/src/errors.ts
Outdated
@@ -18,7 +18,7 @@ export class MalformedPackageJsonError extends Error { | |||
constructor(packageJsonPath: string) { | |||
super( | |||
`There is malformed json in ${packageJsonPath}\n` + | |||
'Fix it, and then run "jest --init"', | |||
'Fix it, and then run "create-jest" again', |
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 catch! I'm just not sure if the new suggestion is right. People using --init
will see it too, right? Also user does not run create-jest
directly. Hm.. Perhaps it's enough to say that the file is malformed?
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.
Sounds good, thanks for the idea!
Does it manage to generate commands for Yarn Classic and Yarn Berry? I'm on mobile and can't remember the details, but the commands are different. Perhaps |
Right, it works. See: yarnpkg/berry#1138 |
Great! Do we need to add something else here (except for changelog update)? |
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.
Thanks. I'm happy with everything.
I updated the changelog with the new package |
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.
great stuff, thanks!
Reference regarding the CI failures on Node.js 20.6: nodejs/node#49497 |
f24ea7f
to
9e3ddfa
Compare
Very cool, thanks for building this! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Jest already has jest
--init
wizard which makes initial setup very fancy, but it doesn't have starter-kit wrapper for package managers, like npm init or yarn createDetails in this issue
Test plan
I checked the approach on my own package on
yarn
,npm
,pnpm
(see usage increate-jest
readme), but I don't have access tocreate-jest
package, so I'm not able to test it completelyBut just in case I also checked JS-API of the packed package like this
Also not sure if it's worth to add e2e for this package, please ping me if it is 🙂