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

Updated yarn add in README.md #1500

Closed
wants to merge 1 commit into from
Closed

Updated yarn add in README.md #1500

wants to merge 1 commit into from

Conversation

s0h311
Copy link

@s0h311 s0h311 commented Aug 6, 2023

What?

updated the installation command for yarn.

Why?

For dev dependency it should be yarn add -D and not just yarn add

How?

Testing?

New Dependencies?

Screenshots

Suggested Reading?

Anything Else?

Checklist

  • I read CONTRIBUTING.md.
  • I read MAINTAINERSHIP.md#pull-requests.
  • I added/updated unit tests for my changes.
  • I added/updated integration tests for my changes.
  • I ran the integration tests.
  • I tested my changes in Node, Deno, and the browser.
  • I viewed documents produced with my changes in Adobe Acrobat, Foxit Reader, Firefox, and Chrome.
  • I added/updated doc comments for any new/modified public APIs.
  • My changes work for both new and existing PDF files.
  • I ran the linter on my changes.

@thm76
Copy link

thm76 commented Nov 26, 2023

I think this needs to be more nuanced because in some cases you will need to install pdf-lib as a dependency (I'm generating PDFs inside a React App) but in other cases it's a devDependency - I have another app where I'm generating PDFs at build time only.

In my opinion I would leave it as is, no "-D" flag. This is also consistent with the npm install command.

If the "-D" flag should be added then add it to npm install as well.

@s0h311 s0h311 closed this Nov 27, 2023
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.

2 participants