Skip to content
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

[chrome-launcher] Publish type definitions instead of source TypeScript files #2898

Merged
merged 5 commits into from
Aug 14, 2017

Conversation

devrelm
Copy link
Contributor

@devrelm devrelm commented Aug 9, 2017

This should fix #2896 and reduce errors caused by different ts versions, ts configs, and ts declaration file versions.

In general, this PR enables the option in chrome-launcher's tsconfig to produce declaration files and updates the .npmignore to ignore .ts files.

One additional change was the addition of the @types/rimraf package. This had to be done, as the compiler was complaining about exporting a private type.

@devrelm devrelm force-pushed the devrelm/type-definitions branch from 094d879 to 0368c62 Compare August 9, 2017 03:57
@patrickhulce
Copy link
Collaborator

I agree with publishing the .d.ts files instead 👍 consumers shouldn't have to deal with our typescript compilation quirks. Thanks for taking this on @devrelm!

@patrickhulce patrickhulce requested a review from samccone August 9, 2017 17:16
@samccone
Copy link
Contributor

samccone commented Aug 9, 2017

lgtm. thanks!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

looks like formatting needs to be done for this change. Can fix with cd chrome-launcher && yarn format (and can check locally that it worked with yarn test-launcher-formatting)

@devrelm
Copy link
Contributor Author

devrelm commented Aug 9, 2017

Yeah, I actually noticed that my change to .npmignore would ignore the d.ts files, so I've got a couple changes to make.

I wasn't seeing the formatting error locally, but I actually realized that it's because of a similar globbing issue: the current *.ts and **/*.ts globs used to pick up files for formatting were picking up d.ts files as well. I wasn't seeing the issue locally because I had removed them.

@devrelm devrelm force-pushed the devrelm/type-definitions branch from 0368c62 to dcb9474 Compare August 10, 2017 02:39
@devrelm
Copy link
Contributor Author

devrelm commented Aug 10, 2017

@brendankenny I just added a change to ./chrome-launcher/test/check-formatting.sh to not check the formatting of *.d.ts files. Globs don't quite cut it for selecting files that match *.ts, but not *.d.ts, so I switched it to use find instead. Let me know if you can think of a better way.

I've also updated my commit that touched .npmignore to (hopefully) make sure that d.ts files will be included as well.

@devrelm
Copy link
Contributor Author

devrelm commented Aug 10, 2017

So one final thought: I should probably also update the chrome-launcher format package.json script. It's similarly using *.ts and **/*.ts file expansion globs, so it'll be erroneously formatting *.d.ts files as well.

I see two options here:

  1. make a similar change, calling find, in which case I'd probably just make a function that could be shared between the format and test scripts, so that we only have to make future updates in one place.

  2. use the --glob option for clang-format, which uses node-glob and so has more options to exclude files.

I'll go for the second option for now since it'll be quick, but I'll be happy to change it if you have a different preference.

EDIT: I was mistaken. node-glob doesn't have any more options to negate patterns than standard shell file-expansion. I'll go ahead and implement the first option.

@devrelm
Copy link
Contributor Author

devrelm commented Aug 10, 2017

Alright. I just did the first option, though I didn't make a shared function. Apparently importing scripts with a relative path is not a solved problem in bash, and I don't feel like going down that rabbit hole right now.

@devrelm devrelm force-pushed the devrelm/type-definitions branch from cc98521 to 0365137 Compare August 10, 2017 20:19
@brendankenny
Copy link
Member

@samccone can you take another look?

@brendankenny brendankenny dismissed their stale review August 10, 2017 23:09

over to sam

@devrelm
Copy link
Contributor Author

devrelm commented Aug 14, 2017

@samccone I ran npm pack on this locally on it appears to include the d.ts files and exclude the typescript files. I'm not sure if you want to sanity-check it yourself as well.

Copy link
Contributor

@samccone samccone left a comment

Choose a reason for hiding this comment

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

Thanks for making this fix! LGTM

@samccone samccone merged commit 391e204 into GoogleChrome:master Aug 14, 2017
paulirish pushed a commit to GoogleChrome/chrome-launcher that referenced this pull request Aug 29, 2017
…pt files (GoogleChrome/lighthouse#2898)

* install rimraf types

* enable outputting declaration files

* disable publishing typescript files

* do not check formatting of d.ts files

* do not format d.ts files
@devrelm devrelm deleted the devrelm/type-definitions branch September 16, 2021 15:12
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.

TypeScript undefined-argument error in chrome-finder.ts
4 participants