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(publish): support "publishConfig.directory" field #3490

Merged
merged 1 commit into from
May 31, 2021

Conversation

PabloSzx
Copy link
Member

Adding support for "publishConfig.directory" field, feature present in changesets changesets/changesets#428, and lerna: https://github.com/lerna/lerna/tree/main/commands/publish#publishconfigdirectory

@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch 2 times, most recently from a6c5e59 to d3e4982 Compare May 30, 2021 19:31
@PabloSzx PabloSzx marked this pull request as ready for review May 30, 2021 19:36
@PabloSzx PabloSzx requested a review from zkochan as a code owner May 30, 2021 19:36
@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch from d3e4982 to 9e5402f Compare May 30, 2021 19:37
Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

Please add a test

packages/run-npm/src/index.ts Outdated Show resolved Hide resolved
packages/run-npm/src/index.ts Outdated Show resolved Hide resolved
packages/plugin-commands-publishing/src/publish.ts Outdated Show resolved Hide resolved
@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch 3 times, most recently from 1f20516 to 31bc2e6 Compare May 30, 2021 20:20
@PabloSzx
Copy link
Member Author

PabloSzx commented May 30, 2021

@zkochan I facing an issue, I was able to test this feature, and it works by looking at the logs (since npm execution stdio is being inherited), but I'm not able to spy that stdout programmatically, I did a search online and I couldn't find anything about inspecting an stdio inherit process

image

image

@zkochan
Copy link
Member

zkochan commented May 30, 2021

after publishing the package, install it and check the contents. There are similar tests already in the project.

@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch from 31bc2e6 to 0a92b17 Compare May 30, 2021 22:01
@PabloSzx
Copy link
Member Author

@zkochan I tried following some of the other tests doing the install and I wasn't able to make it work, I give up 😢, the feature is certainly doing it's job, the package in the publishConfig.directory is being published as it can be seen in the logs

@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch 2 times, most recently from 73f9a69 to 5251b68 Compare May 30, 2021 22:13
@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch from 5251b68 to db1e830 Compare May 30, 2021 22:20
@PabloSzx PabloSzx force-pushed the support-publishConfig-directory branch from db1e830 to f8315b8 Compare May 30, 2021 23:44
@PabloSzx PabloSzx requested a review from zkochan May 30, 2021 23:45
@zkochan zkochan merged commit 724c5ab into pnpm:main May 31, 2021
@PabloSzx
Copy link
Member Author

PabloSzx commented Jun 2, 2021

While testing this feature in v6.7.0 I realized there is something wrong, the cwd is set correctly, but not the npm publish arg 😢

This is weird, because in the tests everything worked as intended 🤔

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