Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Minor script updates and quality of life improvements #1853

Merged
merged 5 commits into from
Oct 26, 2023
Merged

Conversation

sdadn
Copy link
Contributor

@sdadn sdadn commented Oct 23, 2023

Summary

This PR makes the following changes:

- Renames the wdio scripts for consistency

The sub-scripts can now be called as wdio:default instead of wdio-default. To prevent bloat in this PR, the wdio scripts at the package level will be updated in a follow up PR.

- Adds a no-coverage jest script

To help speed up testing and prevent boated output when focusing on specific tests. This was also added to the CICD to optimize the build process as the coverage report is meant to be human-readable and has no impact on the output.

- Generates the package-lock.json file

To help optimize the npm install process both locally and in the CICD.

- Updates the Dockerfile to npm 9

It was using the default npm v6.

- Adds a new no-warnings sub-script for scss

This script was added to suppress warnings as it was inflating output and making it difficult to spot errors.

Before:
CleanShot 2023-10-23 at 15 26 55

After:

CleanShot 2023-10-23 at 15 26 17


Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

This was tested by successfully running the scripts.

Jest output

CleanShot 2023-10-23 at 13 01 02

Stylelint output

CleanShot 2023-10-23 at 15 42 31


Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review
  • Tech review

The Dockerfile changes will need to be tested by running a devcontainer and testing the npm version.

Additional Details


Thank you for contributing to Terra.
@cerner/terra

@sdadn sdadn self-assigned this Oct 23, 2023
@github-actions github-actions bot temporarily deployed to preview-pr-1853 October 23, 2023 22:06 Destroyed
@supreethmr
Copy link
Contributor

  • Generates the package-lock.json file
    To help optimize the npm install process both locally and in the CICD.

Package.lock would need to be re-generated and committed back to repo every time the component that is dependent on is updated. I think this will be a additional step for existing projects and might require additional efforts to maintenance.

@sdadn
Copy link
Contributor Author

sdadn commented Oct 24, 2023

  • Generates the package-lock.json file
    To help optimize the npm install process both locally and in the CICD.

Package.lock would need to be re-generated and committed back to repo every time the component that is dependent on is updated. I think this will be a additional step for existing projects and might require additional efforts to maintenance.

What maintenance concerns do you have? This will automatically be added/updated whenever we do npm install (which ideally should happen at least once per story) and we just need to commit an potential changes. If there are conflicts, then we would simply need to delete the file and run npm install again.

In return, we'll get:

  • Faster install times both locally and on the CICD.
  • More consistent dev environment across all engineers.

@sdadn sdadn marked this pull request as ready for review October 24, 2023 14:57
@supreethmr
Copy link
Contributor

supreethmr commented Oct 25, 2023

  • Generates the package-lock.json file
    To help optimize the npm install process both locally and in the CICD.

Package.lock would need to be re-generated and committed back to repo every time the component that is dependent on is updated. I think this will be a additional step for existing projects and might require additional efforts to maintenance.

What maintenance concerns do you have? This will automatically be added/updated whenever we do npm install (which ideally should happen at least once per story) and we just need to commit an potential changes. If there are conflicts, then we would simply need to delete the file and run npm install again.

In return, we'll get:

  • Faster install times both locally and on the CICD.
  • More consistent dev environment across all engineers.

We had scenario where we had made fix on terra-arrange as part of impact testing we performed testing of terra-menu and other components of terra-framework by having tar.gz file of terra-menu. since we had changed the version to refer to tar.gz file version package.lock file was re-generated on local and the fix was working as expected but this fix was not available on terra-ui even after a week of terra-arrange release. Cause for fix not being available on terra-ui is package.lock file of terra-framework which was still referring to an older version of terra-arrange. So we were required open-up a PR update package.lock manually to pull in the latest version. ( so this is one of maintenance concern as it is new additional step which will be required on each of those components which has dependent components on different repo ) .

Does this scripts auto updates the package.lock on timely interval to ensure latest versions are being pulled into project.

@sdadn
Copy link
Contributor Author

sdadn commented Oct 25, 2023

@supreethmr

We had scenario where we had ...

Are you sure that the package-lock was the cause? Because that does not sound correct. The package-lock is for the terra-framework repo and Terra UI has no visibility to that. Nor should Terra UI be affected by the terra-framework repo changes as they are two independent repos. I can delete the terra-framework repo and this will have no impact on Terra UI.

What Terra UI does is pull in the terra-framework-docs package and we have disabled package-lock for each individual package. Even if it wasn't, package-lock won't be be published to the npm registery (https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json).

It feels like there was a different cause because Terra UI only has visibility to the terra-framework-docs package and should not be affected by the repo at all.

@supreethmr
Copy link
Contributor

@supreethmr

We had scenario where we had ...

Are you sure that the package-lock was the cause? Because that does not sound correct. The package-lock is for the terra-framework repo and Terra UI has no visibility to that. Nor should Terra UI be affected by the terra-framework repo changes as they are two independent repos. I can delete the terra-framework repo and this will have no impact on Terra UI.

What Terra UI does is pull in the terra-framework-docs package and we have disabled package-lock for each individual package. Even if it wasn't, package-lock won't be be published to the npm registery (https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json).

It feels like there was a different cause because Terra UI only has visibility to the terra-framework-docs package and should not be affected by the repo at all.

I mean terra-framework site : https://engineering.cerner.com/terra-framework/components/terra-abstract-modal/abstract-modal/abstract-modal

And also I would like to know time difference in build time before and after inclusion of package.lock file in repo.

@sdadn
Copy link
Contributor Author

sdadn commented Oct 26, 2023

And also I would like to know time difference in build time before and after inclusion of package.lock file in repo.

@supreethmr

This branch (with package-lock)
CleanShot 2023-10-26 at 14 14 03

Main (without package-lock)
CleanShot 2023-10-26 at 14 14 09

1 min may not seem much overall but we have 20 jobs on our current build. Each job does its own npm install. So that's ~20 min worth of resources we're saving.

@sdadn sdadn merged commit 02bfa15 into main Oct 26, 2023
@sdadn sdadn deleted the misc-script-updates branch October 26, 2023 21:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants