Skip to content

Types #90

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

Closed
wants to merge 34 commits into from
Closed

Types #90

wants to merge 34 commits into from

Conversation

j-maas
Copy link
Contributor

@j-maas j-maas commented Jan 15, 2019

Ports the library to TypeScript.

src/index.ts Outdated
function compile(sources, options) {
var optionsWithDefaults = prepareOptions(options, options.spawn || spawn);
function compileSync(sources: string | string[], options: Partial<Options>): ChildProcess {
var optionsWithDefaults = prepareOptions(options, spawn.sync as any);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Options contain the spawn constructor. This is difficult, because the async versions use a different one than the sync versions, which I couldn't figure out how to type.

My suggestion would be to factor out the spawn constructor from the options and pass it separately.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 we can definitely do some follow up PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. It's probably best to do that in a separate PR, since it looks like a heavier refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@jackfranklin jackfranklin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, a nice step towards making this codebase easier to work with! I've left a couple of comments but otherwise I think we should move ahead with our move into TS.

@jackfranklin
Copy link
Collaborator

@y0hy0h can we introduce an npm command to run type checking that we can run as part of the build?

@j-maas
Copy link
Contributor Author

j-maas commented Mar 22, 2019

@y0hy0h can we introduce an npm command to run type checking that we can run as part of the build?

I am not sure if I understand what you mean. I consciously left out strict: true from the tsconfig.json, since it still spits out a lot of errors that need fixing. You could enable that, if you want.

If that's not what you want, could you explain what you mean? A dry-run compile command?

@j-maas
Copy link
Contributor Author

j-maas commented Oct 10, 2020

Closed in favor of #107.

@j-maas j-maas closed this Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants