Skip to content
This repository has been archived by the owner on Apr 21, 2022. It is now read-only.

Replace custom np implementation with direct usage #54

Merged
merged 6 commits into from
Jun 14, 2019

Conversation

FredKSchott
Copy link
Owner

resolves #51, resolves #48, resolves #44, resolves #41, resolves #20


Originally, @pika/pack forked the popular https://github.com/sindresorhus/np package so that it could support its own custom publish pipeline. Forking was chosen over using the package directly so that we would have complete control of the implementation & interface while working around some limitations in the library (specifically: always assuming the pkg/ sub-directory to be published, and needing to run build after a version bump).

Fast forward to today, and many of the reported issues with this library are related to our custom np fork/implementation. Meanwhile, the np package has continued to improve, fix bugs and add features.

This PR revisits this decision to fork np, and instead calls out to the library directly. This removes ~1000 lines of code from our package, adds new features & bug fixes, and greatly shrinks the complexity of what we need to maintain.

We now address the original issues as follows:

  1. Always pass --contents pkg/ by default to np
  2. Require that you have a "version" lifecycle hook/script in your package.json, so that a new build is triggered after every version bump.

While requiring a version script is annoying, it's a worthwhile tradeoff that also allows for users to bring their own publish pipeline/tools if they don't want to use @pika/pack publish.

@FredKSchott
Copy link
Owner Author

You can try this out today via npm install @pika/pack@next or npx @pika/pack@next build|publish

@FredKSchott FredKSchott merged commit 6647e7c into master Jun 14, 2019
@FredKSchott FredKSchott deleted the remove-builtin-np branch June 14, 2019 22:43
@gr2m
Copy link
Contributor

gr2m commented Jun 15, 2019

Could you point me to the thinking behind making np part of the library instead of documenting how to publish using np after building with pack build? I’m just curious. I don’t use np myself as I automate releases with semantic-release, so I’d rather prefer to have it removed altogether. At first sight, publishing seems to be out of scope for a package building tool? Also I just love minimal tools that do one thing well, I’m very biased that way :)

@tunnckoCore
Copy link

I agree with @gr2m. Let's focus on the building/packaging only.

@FredKSchott
Copy link
Owner Author

So those two points under "We now address the original issues as follows" are still valid:

  1. npm publish publishes the top-level directory, and not pkg/. Users who don't know how the command works (I would guess most people) will need to go out and figure out how to fix this to publish.
  2. We need to build after version bumping (so that package.json version -> pkg/package.json version). How to do this is also not obvious and can lead to some weirdness.

If you have other ideas on how to address those two issues, I'm definitely interested. This PR was actually an attempt to move closer to what you're describing, where we just wrap a recommended tool with good defaults/options instead of trying to build it ourselves. Let's see how this goes and we could always move to wrapping npx np so that theres only a cost when you run it.

@ghengeveld
Copy link

ghengeveld commented Jun 15, 2019

How about using np as an optionalDependency? If it exists, use it, if it doesn't, don't deal with publishing at all.

I'm a big fan of what pika/pack is offering to solve (the multi env build part), but the publish part just doesn't fit in. I've never had problems using npm version & publish directly. However I have been struggling to incorporate pack into a monorepo Lerna + Yarn workspaces setup over the past weeks. Having multiple tools trying to "solve" the publish part is not helping.

In case you're interested, the wip on that is here: async-library/react-async#44

@gr2m
Copy link
Contributor

gr2m commented Jun 16, 2019

I would suggest to create a separate package, e.g. @pika/publish, which addresses both your points

@FredKSchott
Copy link
Owner Author

@ghengeveld awesome, will check out react-async.

@pika/publish would be interesting, as long as it's easy for a beginner to use and we've communicated well that npm publish isn't enough to publish their package. I'd probably still want an npx @pika/publish shortcut in that case :)

@FredKSchott
Copy link
Owner Author

The other thing that this conversation is circling is: are these really just different commands on a single pika interface. So instead of npx @pika/{web|pack|publish}, is this really just pika {web|build|publish}?

@ghengeveld
Copy link

ghengeveld commented Jun 17, 2019

Honestly, pack isn't the best name for a package. I always have to refer to it as "pika pack" which doesn't feel right. I'd be fine with pika as the package name, with pack being one of its commands.

@tunnckoCore
Copy link

tunnckoCore commented Jun 17, 2019

@FredKSchott, I was thinking about that ever. Move the "runner" implementation to @pika/runner or @pika/core and make another @pika/cli for the cli which will include all of the others as commands.

IMO, I still think pack is too much source code than needed, for actually basic resolving and merging of shareable plugins/configs/etc. 😄 But anyway.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
4 participants