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

Documentation: Add additional information about the canvas package #3653

Closed
wants to merge 2 commits into from

Conversation

DanKaplanSES
Copy link
Contributor

  • I removed the information about the canvas package being a "peer dependency" because, as far as I could tell, this was an implementation detail. If I'm right about that assumption, it could confuse users who are unfamiliar with the concept.
  • I reiterated information about the relationship between the canvas package, the <img> element, and the ResourceLoader. I believe this redundancy is warranted because users could spend a lot of time troubleshooting if they miss this detail.

- I removed the information about the canvas package being "peer dependency" because, as far as I could tell, this was an implementation detail. If I'm right about that assumption, it could confuse users who are unfamiliar with the concept, leaving them in a state of uncertainty.
- I reiterated information about the relationship between the canvas package, the `<img>` element, and the `ResourceLoader`. I believe this redundancy is warranted because users could spend a lot of time if they miss this detail.
@DanKaplanSES DanKaplanSES changed the title Add additional information about the canvas package Documentation: Add additional information about the canvas package Jan 2, 2024
@DanKaplanSES
Copy link
Contributor Author

Not sure how to rerun this but the failure doesn't seem related to my change? LMK if I'm wrong about that.

@domenic
Copy link
Member

domenic commented Jan 3, 2024

  • I removed the information about the canvas package being a "peer dependency" because, as far as I could tell, this was an implementation detail

This is not correct; a peer dependency is a well-documented concept in the npm ecosystem.

@domenic domenic closed this Jan 3, 2024
@DanKaplanSES
Copy link
Contributor Author

DanKaplanSES commented Jan 3, 2024

  • I removed the information about the canvas package being a "peer dependency" because, as far as I could tell, this was an implementation detail

This is not correct; a peer dependency is a well-documented concept in the npm ecosystem.

Hi @domenic Apologies for how I worded that; I didn't mean to imply peer dependencies are an obscure topic. Though, maybe I don't understand them as well as I thought I did. To be honest, I just learned about them today by reading this arti... Annnnd I just noticed you're the author. This is surreal. (It totally reminds me of a Futurama reference.)

Anyway, before I go, may I ask a clarifying question about the jsdom readme? It says:

To make this work, you need to include canvas as a dependency in your project, as a peer of jsdom.

But how does that bold part affect my behavior differently than if it was written like this:

To make this work, you need to include canvas as a dependency in your project.

Thanks for your time!

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.

None yet

2 participants