Skip to content

chore: Fix package build scripts for git-deps and npm-link symlinks #746

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

Merged
merged 8 commits into from
Mar 16, 2022

Conversation

zashraf1985
Copy link
Contributor

@zashraf1985 zashraf1985 commented Mar 9, 2022

Background

This PR is copied from #481. It was contributed by @jasonkarns a couple of years ago. I just pulled his changes, merged master and resolved conflicts.

Summary

Main change: prepublishOnly is the wrong lifecycle script for building/compiling. prepublishOnly does not run during install for git-deps; nor does it run when a package is being symlinked via npm-link. The correct script is prepare.

This PR:

  • ensures the build/bundle/compile script is accurately defined as build so as to be run properly by the top level lerna run build script.
  • ensures "clean" is run before building
  • ensures clean is defined and exposed such that lerna run clean works properly
  • ensures build is invoked during prepare lifecycle hook (packing, publishing, git-installing, npm-linking)
  • preserves npm test being run during prepublishOnly
  • adds package tarball pattern to gitignore so npm-pack can be run for inspection
  • removes unnecessary files from package.json#files array (license, changelog, readme, and package.json files are included automatically)
  • renames CHANGELOG.MD to be cased properly

Test plan

No code changes

jasonkarns and others added 7 commits May 8, 2020 17:16
`prepublishOnly` is an old lifecycle hook created to aid in the
transition from prepublish to prepare.

Documentation: https://docs.npmjs.com/misc/scripts#prepublish-and-prepare

TL;DR: prepare runs before packing and on bare npm-installs.
That is, in order for a package to do building/compiling when it is
installed as a git-dep (where prepublish _does not run_), one must use
`prepare`. This also applies for when a package is symlinked using
npm-link. In order to ensure the package is built and can be consumed by
the linked dependant, the build must occur via `prepare`.

It's also worth noting that `prepare` runs _before_ `prepublish`; so the
build step is guaranteed to be complete prior to the prepublish script;
for, say, running tests.
The top-level package defined clean and build scripts that were expected
to run the (surprise!) clean and build steps of each package. However,
most packages named their build step 'tsc' instead of 'build', and
virtually none of them had 'clean' as a separate step.
It's frequently necessary to run npm-pack when validating the package
tarball is created correctly. This ensures the tarballs are themselves
ignored from git.

(Keeping user-specific ignores, but those shouldn't be in a repo ignore.
User-specific ignores that are OS or IDE specific should be in the
user's own ignore file.)
LICENSE, CHANGELOG, README and package.json files are included in
tarball automatically.
* master:
  Leverage rollup's --config* feature for choosing bundles (#477)
# Conflicts:
#	packages/datafile-manager/package.json
#	packages/optimizely-sdk/package.json
@coveralls
Copy link

coveralls commented Mar 9, 2022

Coverage Status

Coverage remained the same at 97.184% when pulling 1f72078 on zeeshan/npm-scripts-cleanup into 8eedd63 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.184% when pulling f4c0a4f on zeeshan/npm-scripts-cleanup into 8eedd63 on master.

@zashraf1985 zashraf1985 changed the title Fix package build scripts for git-deps and npm-link symlinks chore: Fix package build scripts for git-deps and npm-link symlinks Mar 9, 2022
@zashraf1985 zashraf1985 marked this pull request as ready for review March 9, 2022 19:02
@zashraf1985 zashraf1985 requested a review from a team as a code owner March 9, 2022 19:02
@zashraf1985 zashraf1985 removed their assignment Mar 9, 2022
Copy link
Contributor

@opti-jnguyen opti-jnguyen left a comment

Choose a reason for hiding this comment

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

@zashraf1985

  • Optimizely-sdk/package.json may not need some default files in configuration, such as README and package.json

Otherwise LGTM!

@zashraf1985 zashraf1985 merged commit 5d526bd into master Mar 16, 2022
@zashraf1985 zashraf1985 deleted the zeeshan/npm-scripts-cleanup branch March 16, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants