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

add readme to publishBody #5374

Merged
merged 12 commits into from
May 16, 2023
Merged

Conversation

hindicator
Copy link
Contributor

@hindicator hindicator commented Apr 7, 2023

What's the problem this PR addresses?

Add readme field to publish body so registry would pull this.

How did you fix it?
Read the README.md file from currentDir and add to publish body.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@@ -0,0 +1,48 @@
import {httpUtils} from '@yarnpkg/core';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Few things that would really help me -

  1. About tests :
  • I have two ideas how to implement:
  • e2e test with makeTemporaryEnv, spyOn npmHttpUtils.put and expect to be called with body that contain readme.
  • unit test and builders, build required params for makePublishBody aka
    workspace: Workspace, buffer: Buffer, {access, tag, registry, gitHead}: PublishAdditionalParams
    such as inside builders.ts
    export function aWorkspace(): Workspace{}
    and just call this function.

I think the first option is better but I can't seem to succeed implement a spyOn.
any suggestions ?

  1. I believe readmeFilename is deprecated but -
    see this -
    json: readmeFilename npm/npm#3573 (comment)
    and this -
    Set readmeFilename field on root metadata npm/cli#4252

Do you think we need to add this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also should add test for workspace(monorepo) as the readme file is read with cwd,
I don't know if cwd is relative when using yarn workspaces foreach npm publish

@hindicator hindicator dismissed a stale review via e24f4b9 April 11, 2023 22:01
@arcanis arcanis merged commit 77b8b52 into yarnpkg:master May 16, 2023
merceyz pushed a commit that referenced this pull request May 16, 2023
* add readme to publishBody

* clean test with example

* add fallback to project name

* cleaning

* fix getPublishAccess

* cleaning

* remove unused parameter

* Tweaks

* Tweaks tests

* Versions

* Update publish.test.ts

---------

Co-authored-by: Maël Nison <[email protected]>
merceyz pushed a commit that referenced this pull request May 16, 2023
* add readme to publishBody

* clean test with example

* add fallback to project name

* cleaning

* fix getPublishAccess

* cleaning

* remove unused parameter

* Tweaks

* Tweaks tests

* Versions

* Update publish.test.ts

---------

Co-authored-by: Maël Nison <[email protected]>
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