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

Check types in typescript packages in CI #780

Merged
merged 6 commits into from
Feb 14, 2021
Merged

Check types in typescript packages in CI #780

merged 6 commits into from
Feb 14, 2021

Conversation

fernandopasik
Copy link
Contributor

@fernandopasik fernandopasik commented Jan 29, 2021

Rollup Plugin Name: alias, auto-install, data-url, eslint, html, pluginutils, typescript, virtual, wasm

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no (adds tasks to projects and ci)

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
fixes #779

Description

Have early feedback in every PR that typescript types are not compromised.
Add task test:ts to every plugin written in typescript
Run test:ts on circle ci config

@fernandopasik fernandopasik changed the title Check types ci Check types in typescript packages in CI Jan 29, 2021
Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

I think "check" or "typecheck" would be a better script name

.circleci/config.yml Outdated Show resolved Hide resolved
@fernandopasik
Copy link
Contributor Author

Hi @NotWoods, after a bit more thought I asked some question about this approach here #779 (comment)

I think I'm missing context on why is it necessary to commit the types/index.d.ts when the source is already typescript. I was expecting that file to be generated along side the js files before publishing to npm only. @shellscape Do you have more information on this?

I don't think the test:ts task is checking well the types. I checked this reverting locally the changes in #647 and it doesn't catch the error.
It did catch it if I directly run tsc --noEmit, because that runs on all ts files of the plugin.

@shellscape
Copy link
Collaborator

So I know that historically we've had the the index.d.ts files hand-written before plugins were moved to TypeScript. That made an additional test of the types necessary. Some of those have stuck around. We even have some TypeScript-sourced plugins (maybe they're all like this still?) that have a separate, hand-written index.d.ts, which again is important to check against the code.

In the case of plugins which are already switched over to TypeScript sources, we should probably be generating the definitions at build time, and then removing the additional type check tests (as you point out, it'd be unnecessary). If we do that, for any plugin which uses generated definitions rather than the hand-written definition, we'll have to do a separate PR for each plugin.

@fernandopasik
Copy link
Contributor Author

Ok, sounds good @shellscape. But for the scope of this PR I managed to deliver the type checking by checking all the ts files in each package, not just the index.d.ts like before.

@NotWoods I've merged your proposed change.

@shellscape shellscape merged commit 1e14664 into rollup:master Feb 14, 2021
@fernandopasik fernandopasik deleted the check-types-ci branch February 14, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check types in CI for all plugins written in ts
3 participants