feat(api): proper esm support and codegen overhauls #754
Conversation
erunion
changed the title
erunion
added
enhancement
| * `ts-morph` unfortunately doesn't give us any options for programatically doing this | ||
| * so we need to resort to modifying the emitted JS code. | ||
| */ | ||
| code = code |
There was a problem hiding this comment.
goodbye forever to this mess
| const sourceFile = this.project.createSourceFile('schemas.ts', ''); | ||
| const schemasDir = this.project.createDirectory('schemas'); | ||
| private createSchemasFile(sourceDirectory: Directory) { | ||
| const sourceFile = sourceDirectory.createSourceFile('schemas.ts', ''); |
There was a problem hiding this comment.
These sourceDirectory changes mean that these files we're creating here (schemas.ts and schemas/) are now being created in a src/ directory.
erunion
force-pushed
the
refactor/codegen
branch
from
9f45fd2 to
9fd0370
Compare
* chore: minor tsconfig cleanup * fix: point to correct api-core pkg version * fix: install TS as dev-dep * fix: add this to silence the JSON import warning * refactor: slight cleanup
packages/api/package.json
Outdated
| @@ -54,12 +55,12 @@ | |||
| "prompts": "^2.4.2", | |||
| "semver": "^7.3.8", | |||
| "ssri": "^10.0.1", | |||
| "ts-morph": "^17.0.1", | |||
| "ts-morph": "^20.0.0", | |||
| "tsup": "^7.2.0", | |||
There was a problem hiding this comment.
could we remove this as a dep since it's being installed in the codegen'd directory now?
| requiredPackages!: Record< | ||
| string, | ||
| { | ||
| reason: string; |
There was a problem hiding this comment.
as a follow-up to the work I added in #759, i was thinking it might make sense to have a devDep flag that splits out our required deps (e.g., @readme/api-core) from our required dev-deps (e.g., typescript)
| reason: string; | |
| devDependency: boolean; | |
| reason: string; |
There was a problem hiding this comment.
will do this in another PR since it's going to require a little bit of work
There was a problem hiding this comment.
might wanna regenerate these now that this file has devDependencies
| // }); | ||
| // }); | ||
|
|
||
| it('should be able to make an API request (JS + ESM)', async () => { |
There was a problem hiding this comment.
what if we added a test in this file that imports the generated cjs file and ensures that that works?
There was a problem hiding this comment.
thats going to require building a dist for these fixtures and keeping that dist in git. i want to move these fixtures into @api/test-utils at some point, will rework this then
| } | ||
| : {}), | ||
| }, | ||
| dependencies, |
There was a problem hiding this comment.
per this comment, might be good to add the dev-deps here so we're not running npm i several times
| async compile(storage: Storage, opts: InstallerOptions = {}): Promise<void> { | ||
| const installDir = storage.getIdentifierStorageDir(); | ||
|
|
||
| await execa('npm', ['install', 'tsup', 'typescript', '-D'], { |
There was a problem hiding this comment.
per this comment, might be good to remove this and take care of the dev-dep installation in the earlier install command so we're not running npm i several times
This reverts commit 3b82df8.
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
🧰 Changes
api installto no longer prompt for a language or target to install. JS is now the only export target.ts-morphhacks that didn't really support ESM environments.src/directory.dist/directory post-tsupcompilation.ts-morphto the latest release. chore(deps): bump ts-morph from 17.0.1 to 20.0.0 #752🧬 QA & Testing
.cjs,.mjs,.js, and.tsexample file?