fix: package builds respect --private option #1433
Conversation
🦋 Changeset detectedLatest commit: e38289a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Created a changeset, will merge once checks have completed again. |
|
@benpryke Although, just seen there's no test to cover this case. Can we add one? |
|
I tried running the test suite locally with no changes, and a large number of tests fail. I think about 10 suites and 55 individual tests failed altogether. Specifically, the test suite that I wanted to add to, build.test.ts fails completely. I tried clean install of node_modules just to confirm I didn't have a dirty state and had the same experience. Is some additional setup required? I couldn't see any docs to suggest that. If this is actually broken right now, I'd rather not fix the world for a one-line patch, but I don't disagree that there's a gap for a couple of cross-package build tests. |
Hi Ben, unfortunately my laptop is out of order atm and I'm waiting for it to be fixed so I can't try, but for me no additional step was required to run the tests, apart from the usual Could you try to tun them with |
|
@benpryke make sure you're running |
| if ( | ||
| includePrivate || | ||
| packageJsons[importedPackage].private !== true | ||
| ) { | ||
| localImports[importedPackage] = packageJsons[importedPackage] |
There was a problem hiding this comment.
Maybe let's add a console warning here if we are in the case where we includePrivate and also hit this branch?
There was a problem hiding this comment.
I think it's perfectly reasonable to break projects down into multiple packages that it wouldn't make sense to publish on their own, so I don't think a warning is warranted every time someone builds a project that relies on unpublished packages in this way. Maybe you're thinking about this in a different way?
There was a problem hiding this comment.
The use case of building a package for publishing implies that all the dependencies are being published and maybe worth considering if a packages should be public or not. I wouldn’t expect anyone to be using the private flag for building on packages which aren’t being published.
Given that private package building is opt in I would put a warning to show that the package being built is depending on other pieces which would need to be build opt in.
There was a problem hiding this comment.
I think there's a distinction between publishing the source and building a distribution. The private field is simply a way of specifying that a package should not be published standalone and implies nothing about the build process or how it's otherwise used.
I presume users will most commonly add the private flag after encountering a build failure where they used private package dependencies in their project.
Given the above, it still strikes me as odd to repeatedly display a warning for something the user has opted into.
There was a problem hiding this comment.
I'd suggest we merge as-is and split out a new feature of debug logging that would be useful for notice kind of messages like this.
|
LGTM apart from my one comment. |
No description provided.