Skip to content
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

[WIP] Remove build artifacts from the repo #295

Merged
merged 6 commits into from
Oct 29, 2018

Conversation

dmethvin-gov
Copy link
Contributor

@dmethvin-gov dmethvin-gov commented Oct 4, 2018

Fixes #218

This is a breaking change since potentially some people might be expecting the /lib contents to be there on each build. A couple of things:

  • I have questions about where the images and fonts should go, they were in /lib but aren't copied there? I think our build process needs to copy anything we require or we need to figure out a better way to reference them. (This is the subject of Discovery on best way to import USWDS images and fonts #66 I think but I wanted to be sure I understood all the issues here.)

  • As far as leaving things out, should we remove our eslint and babel configs from the published package? It depends on whether we expect people to transpile our code, there are a bunch of pro/cons but I was thinking it might be good to leave it in for now.

Once those are resolved I think it should be possible to land this. I ran a npm publish --dry-run and it seemed to put in the right files.

Types of changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've reviewed the guidelines for contributing to this repository.
  • I've checked style and lint with npm run lint.
  • I built the production version with npm run build.
  • I added tests to verify a bug fix or new functionality.
  • All tests pass locally with npm test.
  • My change requires a change to the documentation, and I've updated the documentation accordingly.

@annekainicUSDS
Copy link
Contributor

@dmethvin-gov to answer your questions:

  1. Yes, those images and fonts were manually copied directly into the /lib directory once. Not at all ideal, but that's what that ticket you referenced is meant to look into. I assume we would need to address that before this PR could be merged?

  2. I think we decided at the beginning of the project to not assume that users will do their own transpilation, and for us to include the transpiled files. If we aren't committing these files, when are they going to be created and how will they be used? I'm a little unclear on how this change to remove build artifacts completely works with our current assumptions about how users import files from USFS into their own repos, and what would need to change.

@dmethvin-gov
Copy link
Contributor Author

On 1, if that's the way we want it to stay we can just copy them into /lib when npm run build happens. I was trying to figure out a way to avoid pulling in the entire USWDS since it's a 181KB CSS file but I'm not sure how to do that since our caller may need things we don't use directly. We've got USWDS listed as a peerDependency but if we are packaging it all in our lib it's really not a dependency of our caller, but just us.

On 2, all of the files are always going to be built and linted on each commit to master or on PRs, since they're needed to run tests. It's just a question of whether /lib is saved or not after that is done. Right now we're saving all that to the repo, but with this PR we'd just save it when we publish. The new .npmignore takes care of that.

@dmethvin-gov
Copy link
Contributor Author

Looking at this some more I think we should have this PR resolve #66 along with #218. Some thoughts about it:

  • If we copy just some of the USWDS /dist files we need into locations outside node_modules and check them into our repo and/or publish our package with them, we should also be copying the license info for it and mention their license in ours.
  • We can avoid those complications if we could use the files directly out of node_modules, at that point it could be a true peerDependency. Right now the starter app is using what it does use in USWDS through us, which means even patch versions of USWDS would require us to cut a new release.
  • I wonder how others are using USWDS via npm, and if so what does their setup look like? The installation instructions seem to encourage npm and don't talk about it being copied.

@dmethvin-gov
Copy link
Contributor Author

@annekainicUSDS I think this is ready for another look. I rebased against the current master in order to remove some of the merge conflicts with the built files.

Copy link
Contributor

@annekainicUSDS annekainicUSDS left a comment

Choose a reason for hiding this comment

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

Couple more questions for you!

package.json Outdated
"autoprefix": "npx postcss lib/css/styles.css --use autoprefixer -map -d lib/css/",
"build": "npm run compile-js && npm run compile-sass && npm run autoprefix && npm run copy-uswds -s",
"ci": "npm run clean -s && npm run lint && npm run build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not want to run npm run test as part of this? Now that we're using this command in our circleci config instead of npm run lint and npm run test, we've lost the command to run the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, we do but I messed up fixing the merge conflict. This was exactly one of those situations where the built files updated causing a merge conflict, . I can add it back. Thanks for catching it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha!

package.json Outdated
"autoprefix": "npx postcss lib/css/styles.css --use autoprefixer -map -d lib/css/",
"build": "npm run compile-js && npm run compile-sass && npm run autoprefix && npm run copy-uswds -s",
Copy link
Contributor

Choose a reason for hiding this comment

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

We've removed the part where we clear out the lib/ directory first before adding the compiling files. Is that not something we still need to do? To make sure that files we may have deleted are being deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For CI it's nice to be completely sure it's clean but I wasn't sure whether it was really needed when you're building over and over locally. Files removed from the project are relatively rare. I can put it in here as well though if you think we should have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can't hurt! Might as well include it to be extra sure it's reflective of reality.

@dmethvin-gov
Copy link
Contributor Author

OK, I moved all the steps into build, including lint. Later we may want to add some less-costly shortcuts because it takes a while to run.

@dmethvin-gov
Copy link
Contributor Author

Once we remove the build artifacts we could also revisit the use of a push hook or commit hook which we couldn't use before.

Copy link
Contributor

@annekainicUSDS annekainicUSDS left a comment

Choose a reason for hiding this comment

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

LGTM!

@dmethvin-gov dmethvin-gov merged commit 1702d1a into usds:master Oct 29, 2018
dmethvin-gov added a commit to dmethvin-gov/us-forms-system that referenced this pull request Oct 29, 2018
@dmethvin-gov dmethvin-gov deleted the 218-artifacts branch December 24, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not commit build artifacts
2 participants