-
Notifications
You must be signed in to change notification settings - Fork 143
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
Support reading information from tsconfig.json #509
Conversation
The glob dependency is added for supporting tsconfig.json
Here we can run `sucrase` and ask it to just read our tsconfig for information instead passing it N CLI arguments. It will read meta data from `tsconfig.json` which includes `files` & `include`. This allows it to just compile our project. Also in case you have an `include` with multiple folders like `src/**/*.ts` and `test/**/*.ts` then you can compile your project in one step. Currently `sucrase` only supports one entry dir instead of multiple entry dirs.
@alangpierce do you have any interest in this feature ? |
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 delay, and thanks for working on this! Getting configuration/tooling right feels like a tricky problem since there are so many use cases to keep in mind. I've read the PR over and thought about it a bit and number of thoughts come to mind.
Immediate thoughts:
- At a high level, the change makes sense and the implementation looks correct from what I can see. Thanks!
- My biggest concern is that it's kind of unclear what the right behavior is if you specify
--project
and also other options that conflict. I think for now I'd prefer to disallow that case rather than trying to merge the options. From an API backcompat standpoint, I'd prefer to make a focused change that can be extended in an additive way. One way to approach it would be the "CLI args" code path and the "tsconfig" code path, with both of them returning a common configuration format that the CLI uses, like anArray<FileInfo>
as part of the options object. Similar to what you have, but I think having an explicit shared options datatype that both forms parse to would be nice. - The
cli.ts
file is getting pretty big with this PR. Maybe you could split out the tsconfig stuff into its own file? Also seems fine to havecli/index.ts
andcli/tsconfig-parser.ts
or something like that. - It feels awkward for
findFiles
to be callingmkdir
on the out directory, since the name "find" suggests to me that it doesn't have any side-effects. One idea is you could remove that part and use mkdirp in thebuildFile
function. - There's some complexity in fully parsing tsconfig files and I worry a little about going down a rabbit-hole like that. At my work, we use
extends
to compose tsconfigs together, so ideally this would support that. At the same time, it seems fine to do the 90% solution for now rather than worrying about getting every detail right.
I also left a few comments inline.
Longer-term thoughts:
- It would be great if this logic could also benefit the webpack plugin, the require hook, etc. For example, something like
updateOptionsFromProject
could be inindex.ts
or something more "core". That would mean having separate functions to parse sucrase options vs CLI options, though maybe that's not a bad thing. - I think ideally we'd use exceptions rather than
process.exit
. In other tools, I've had a special exception type that gets displayed as a user-facing error. But there were existing uses ofprocess.exit
and no great system for reporting user-friendly errors, so seems fine to use more. - Ideally this CLI code would get tests sooner rather than later. But that's my fault for not including them from the start. TBH, I've generally viewed the CLI as sort of a rare use case compared with the require hook or webpack plugin.
I don't consider any of the concerns critical/blocking, though. Let me know what you think! If you want to take a crack at some of it or discuss, that would be great. Otherwise, I'm fine merging this as-is and working from there (I might want to make some follow-up tweaks to options validation before publishing).
if (file.endsWith(".d.ts")) { | ||
continue; | ||
} | ||
if (!file.endsWith(".ts") && !file.endsWith(".js")) { |
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.
Could you have this also check for .tsx
and .jsx
files?
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.
Will fix.
|
||
const compilerOpts = json.compilerOptions; | ||
if (compilerOpts.outDir) { | ||
options.outDirPath = join(process.cwd(), options.project, compilerOpts.outDir); |
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.
Stylistically, I'm not a fan of mutating objects after they're created like this. I think I'd prefer to have this function accept a project path and return a CLIOptions
object or a Partial<CLIOptions>
object.
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'll look at fixing that.
let str; | ||
try { | ||
str = await readFile(tsConfigPath, "utf8"); | ||
} catch (err) { |
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.
Using try/catch feels a little weird here. I think I'd prefer to use exists
and give a friendly message if that fails, and if there's some other error, it can make its way up as a real crash with stack trace.
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.
exists
is deprecated ( https://nodejs.org/api/fs.html#fs_fs_exists_path_callback ). The documentation recommends just trying the readFile
and handling the error instead of the exists
+ readFile
since that's a timing race condition.
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, fair enough. If possible, it would be nice to detect if it's a "file not found" error (maybe err.code === 'ENOENT'
like in the docs) and re-throw if it's an unrecognized error.
(In this case, I'm not convinced that the race condition actually matters, since the worst case is an uglier error message, and there are plenty of similar cross-file race conditions that can't be reasonably handled, but I guess it's nice to make the ugly error impossible.)
|
||
let str; | ||
try { | ||
str = await readFile(tsConfigPath, "utf8"); |
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.
It would be nice to factor the code so we don't need to do this twice.
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.
willfix.
The CLI is my main use case. My current typescript workflow ( https://github.com/Raynos/fake-api-gateway-lambda/blob/master/package.json ) is to use Then I can use vanilla JS tooling of my choice. For example When adopting |
I'll see if I can carve out some time to address feedback. I might loop back in a few days and suggest you merge as is. |
Fair enough. You may be able to configure all of those to compile on-the-fly to avoid the need for a build step (e.g. |
I have not tried The nice thing about running If node supports typescript natively like |
I haven't used tape specifically, but generally with |
Right line numbers are the same, which is why I am using I guess |
I'm not using typescript in my day-to-day atm. I would recommend you to pull it in as-is and make patches to it. I don't want to hold the lock & block you. |
The current
sucrase
binary only supports a singlesrcDir
instead of multiple.My typescript project has a
./src
and./test/
dir that are compiled tobuild/src
&build/test
; to do that with sucrase I have to run sucrase twice.This would lead to something like
With this PR I can just
sucrase -p .
;I also implemented reading
outDir
;transforms
; &legacy module interop
fromtsconfig.json
since that information also exists there.I tested this locally on three typescript projects (
fake-kms
,fake-cloudwatch-logs
,fake-api-gateway-lambda
).