-
Notifications
You must be signed in to change notification settings - Fork 66
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: valid package json on app create #1031
Conversation
7ce2f31
to
75de26c
Compare
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!
@@ -52,6 +52,7 @@ | |||
"http-errors": "^1.7.3", | |||
"lodash": "^4.17.15", | |||
"node-fetch": "^2.6.0", | |||
"parse-json": "^5.0.0", |
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 to improve dx around the bad feedback given by JSON.parse
@@ -72,6 +72,12 @@ export class __Default implements Command { | |||
console.log() // space after codeblock | |||
|
|||
break | |||
case 'malformed_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.
This is a new feature that led to catching the bug.
@@ -533,6 +533,7 @@ async function scaffoldBaseFiles(options: InternalConfig) { | |||
fs.writeAsync(Path.join(options.projectRoot, 'package.json'), { | |||
name: options.projectName, | |||
license: 'UNLICENSED', | |||
version: '0.0.0', |
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.
fixing the bug
@@ -133,7 +133,6 @@ describe('tsconfig', () => { | |||
}, | |||
], | |||
"rootDir": ".", | |||
"skipLibCheck": true, |
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 is reverting a temp feature we had in the release series.
export async function scan(opts?: { | ||
cwd?: string | ||
entrypointPath?: string | ||
}): Promise<Either<Error, ScanResult>> { |
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.
Scan is now an either, since it cares about failed reads of package.json now.
/** | ||
* | ||
*/ | ||
function findPackageJson(opts: { projectRoot: string }): ScanResult['packageJson'] { |
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 have a new lib module for dealing with package json
e.context = ctx | ||
return e | ||
} | ||
} |
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 am not satisfied with the usability but it seems to get the job done which is a two step processes where we:
- define error types
- create instances of those error types
I am open to using classes but didn't see how that would help.
I hope we can make this better in the future.
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 find it interesting as a first iteration and certainly not bad. Let's simply see how that works in practice for now and iterate if it turns out to not work well!
This was intended to be some refactoring leading to #1012
However I discovered a reason to treat this beyond a refactor.
The fix is that version field is a required field of package json, and we were not supplying it during app creation.