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

docs(readme): add types #903

Merged
merged 5 commits into from
Jan 25, 2022
Merged

docs(readme): add types #903

merged 5 commits into from
Jan 25, 2022

Conversation

harish-sethuraman
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Added types in Readme

README.md Outdated
Type:

```ts
type insert = string | ((linkTag: any) => void);
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using any, if you found it in our code, it means bug, we should improve this type

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 linkTag here is an HTMLElement right?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, but mostly yes, just check it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to HTMLLinkElement

README.md Outdated Show resolved Hide resolved
or

```console
yarn add -D mini-css-extract-plugin
Copy link
Member

@alexander-akait alexander-akait Jan 21, 2022

Choose a reason for hiding this comment

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

Change it on npm, because we use npm

Copy link
Member

Choose a reason for hiding this comment

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

If we want to provide yarn, we should also provide pnpm example, i.e. for all package managers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 I've seen npm and yarn in some repos. That is why I started adding yarn in all repos. Should we consider adding pnpm as well?

Copy link
Member

Choose a reason for hiding this comment

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

If we add yarn we should add pnpm too

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #903 (c670345) into master (be75c50) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   90.29%   90.29%           
=======================================
  Files           5        5           
  Lines         824      824           
  Branches      220      220           
=======================================
  Hits          744      744           
  Misses         70       70           
  Partials       10       10           
Impacted Files Coverage Δ
src/index.js 96.27% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fd41e3...c670345. Read the comment docs.

@alexander-akait alexander-akait merged commit 6650691 into webpack-contrib:master Jan 25, 2022
@alexander-akait
Copy link
Member

Thanks

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