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: add fromContent method to set contents directly #96

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 22, 2024

Currently to replace read-package-json-fast in pacote we already have a manifest that needs normalizing. The only way to do that currently is to pass in a string and re-JSON.parse it which is a waste. This allows for content to be set directly.

@@ -167,6 +167,12 @@ class PackageJson {
return this
}

fromContent (data) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this method was ill conceived. In my head it was the implementation that this PR is now providing.

Having it public was probably a mistake, as it should probably also have #canSave set to false if used directly (and load should not use it directly).

That's neither here nor there for this PR but those are the thoughts that this PR makes me have.

Copy link
Member

Choose a reason for hiding this comment

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

fromComment seems to have the same issue. I think that for how we use this it's fine, but some of these public methods could be used in a way that gives weird behavior when folks try to save.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

This is good

@lukekarrys lukekarrys merged commit 17788d0 into main Apr 22, 2024
21 checks passed
@lukekarrys lukekarrys deleted the lk/from-content branch April 22, 2024 20:30
@github-actions github-actions bot mentioned this pull request Apr 22, 2024
lukekarrys pushed a commit that referenced this pull request Apr 22, 2024
🤖 I have created a release *beep* *boop*
---


## [5.1.0](v5.0.3...v5.1.0)
(2024-04-22)

### Features

*
[`17788d0`](17788d0)
[#96](#96) add fromContent
method to set contents directly (#96) (@lukekarrys)

### Chores

*
[`9597b84`](9597b84)
[#94](#94) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`5e20e64`](5e20e64)
[#94](#94) bump
@npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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