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

feat(dsv,dynamic-import-vars,image,legacy,multi-entry,strip,sucrase,url,yaml): add missing typings #898

Merged
merged 20 commits into from
Jul 26, 2021

Conversation

Luke-zhang-04
Copy link
Contributor

@Luke-zhang-04 Luke-zhang-04 commented Jun 3, 2021

Rollup Plugin Name: dsv, dynamic-import-vars, image, legacy, multi-entry, strip, sucrase, url, yaml

This PR contains:

  • bugfix
  • "feature"
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

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

Description

Adds typings for plugins which were missing them, including dsv, dynamic-import-vars, image, legacy, multi-entry, strip, sucrase, url, and yaml.

Copy link

@marcopixel marcopixel left a comment

Choose a reason for hiding this comment

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

I would recommend making the options parameter optional because this will result in an TS error if no own configuration or an empty object is set inside the plugin function.

--- ERROR
plugins: [url()]

--- PASS
plugins: [url({})]

Since a few of the official plugins are using no option parameters in their examples
(url or multi as example) & having unnecessary empty objects just to "fix" typing errors, i think it's better to let the user decide if they want to declare additional options.

packages/dsv/types/index.d.ts Outdated Show resolved Hide resolved
packages/dynamic-import-vars/types/index.d.ts Outdated Show resolved Hide resolved
packages/image/types/index.d.ts Outdated Show resolved Hide resolved
packages/multi-entry/types/index.d.ts Outdated Show resolved Hide resolved
packages/strip/types/index.d.ts Outdated Show resolved Hide resolved
packages/legacy/types/index.d.ts Show resolved Hide resolved
packages/sucrase/types/index.d.ts Outdated Show resolved Hide resolved
packages/url/types/index.d.ts Outdated Show resolved Hide resolved
packages/yaml/types/index.d.ts Outdated Show resolved Hide resolved
@Luke-zhang-04
Copy link
Contributor Author

Of course, why didn't I think of that

@shellscape
Copy link
Collaborator

Thanks for putting in this work. While I haven't had a chance to look it over (yet. I will) I did want to pop in and mention that this will force me to finally get around to improving our publish-release pipeline so it can handle more than one plugin per Pull Request. It'll probably be a week or so, so don't be discouraged if your PR sits for a bit.

packages/dsv/package.json Show resolved Hide resolved
packages/dsv/package.json Show resolved Hide resolved
@Luke-zhang-04
Copy link
Contributor Author

Hm, didn't think you could test types, but I'll look into it.

packages/dsv/package.json Outdated Show resolved Hide resolved
packages/dynamic-import-vars/test/types.ts Outdated Show resolved Hide resolved
@Luke-zhang-04
Copy link
Contributor Author

can someone help me out with the linting process? When I run pnpm run lint it formats files that I haven't touched. Do I just leave these?

image

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.

You should be able to leave out the files that only have lint changes. I'll double check and see if CI passes without it.

@wdfinch
Copy link

wdfinch commented Jul 2, 2021

This is awesome! I use TS for my config so this really helps.

@shellscape shellscape changed the title feat: add missing typings feat(dsv, dynamic-import-vars, image, legacy, multi-entry, strip, sucrase, url, yaml): add missing typings Jul 15, 2021
@shellscape shellscape changed the title feat(dsv, dynamic-import-vars, image, legacy, multi-entry, strip, sucrase, url, yaml): add missing typings feat(dsv,dynamic-import-vars,image,legacy,multi-entry,strip,sucrase,url,yaml): add missing typings Jul 15, 2021
@shellscape
Copy link
Collaborator

@Luke-zhang-04 the repo is now prepared for automatic publishing, and handling more than one plugin per commit. however, it required some refactoring, which has caused conflicts with your branch. could you please pull from upstream/master and resolve the conflicts in your branch?

@shellscape shellscape force-pushed the master branch 13 times, most recently from 486cc69 to 34ba4ce Compare July 16, 2021 14:41
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.

5 participants