rules_js icon indicating copy to clipboard operation
rules_js copied to clipboard

feat: add with_npm_publish_target to disable npm_publish generation

Open SinimaWath opened this issue 1 year ago • 7 comments
trafficstars


Type of change

New feature or functionality (change which adds functionality):

Arg with_npm_publish_target to npm_package to have possibility to prevent generation of {name}.publish target.

For changes visible to end-users

Suggested release notes are provided below:

Added arg with_npm_publish_target to npm_package to have possibility to prevent generation of {name}.publish target.

Test plan

Manual testing:

  • Provide with_npm_publish_target = False to npm_package
  • bazel query "..." | grep publish.

SinimaWath avatar Mar 18 '24 14:03 SinimaWath

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 18 '24 14:03 CLAassistant

Seems like a reasonable idea to me.

@gregmagolan wdyt? wdyt about naming here?

jbedard avatar Mar 19 '24 00:03 jbedard

@gregmagolan Is it ok for you?

SinimaWath avatar Mar 22 '24 12:03 SinimaWath

@SinimaWath I think we should rename this to publishable. WDYT?

I think we should also add a TODO(2.0): switch to false and in v2 we can make the breaking change of not creating this target by default. I think most of the time packages are only used internally and this target is not even necessary. Worst case we can drop the TODO if we decide we want to keep it as true, best case we switch the default in 2.0.

jbedard avatar Mar 25 '24 20:03 jbedard

@jbedard

Looks good to me, changed that and added TODO

SinimaWath avatar Mar 27 '24 20:03 SinimaWath

I proposed this back when we added the .publish target, but decided it was unnecessary. I don't think much changed since then. What would be justification for adding this other than making the query result smaller?

thesayyn avatar Mar 27 '24 21:03 thesayyn

@thesayyn

Yes, you're right, there are 2 reason for me: 1 - Make query result smaller 2 - Replace name.publish with our internal implementation, but keeping DX the same as using npm_package, so developers don't need to learn a new target of publishing packages. I think it's small changes, but essential.

SinimaWath avatar Mar 27 '24 21:03 SinimaWath

Test

1 test target passed

Targets
//docs:update_6_test [k8-fastbuild] 117ms

Total test execution time was 117ms. 210 tests (99.5%) were fully cached saving 43s.


Test

e2e/bzlmod

All tests were cache hits

4 tests (100.0%) were fully cached saving 1s.


Test

e2e/gyp_no_install_script

All tests were cache hits

3 tests (100.0%) were fully cached saving 930ms.


Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 5s.


Test

e2e/npm_link_package

All tests were cache hits

2 tests (100.0%) were fully cached saving 3s.


Test

e2e/npm_link_package-esm

All tests were cache hits

2 tests (100.0%) were fully cached saving 3s.


Test

e2e/npm_translate_lock

All tests were cache hits

2 tests (100.0%) were fully cached saving 481ms.


Test

e2e/npm_translate_lock_empty

All tests were cache hits

2 tests (100.0%) were fully cached saving 301ms.


Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 435ms.


Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 188ms.


Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 301ms.


Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 393ms.


Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 239ms.


Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 927ms.


Test

e2e/pnpm_lockfiles

All tests were cache hits

4 tests (100.0%) were fully cached saving 440ms.


Test

e2e/pnpm_workspace

All tests were cache hits

8 tests (100.0%) were fully cached saving 7s.


Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

6 tests (100.0%) were fully cached saving 5s.


Test

e2e/repo_mapping

All tests were cache hits

2 tests (100.0%) were fully cached saving 490ms.


Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 2s.


Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 229ms.


Buildifier      Format

aspect-workflows[bot] avatar Apr 23 '24 03:04 aspect-workflows[bot]