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

Update assets #825

Merged
merged 9 commits into from
Jun 13, 2018
Merged

Update assets #825

merged 9 commits into from
Jun 13, 2018

Conversation

faustinoaq
Copy link
Contributor

Description of the Change

This PR does:

  1. Update main package.json depedencies
  2. Recompile amber.min.js with latest dependencies
  3. Update dependencies on package.json.ecr template
  4. Recompile default CSS && JS bundles provided with amber new and latest dependencies

Alternate Designs

No, this change is required by upcoming amber watch configuration

Benefits

Fixes most NPM package vulnerabilities and prepare asset stuff for next PRs

Possible Drawbacks

No

@faustinoaq
Copy link
Contributor Author

BTW, this is still using with webpack v3, I'm already working on someupdates and changes for this on next PRs, although, this PR is required first 😅

@robacarp
Copy link
Member

Since .min.js and .bundle.js files are essentially compiled, why are we tracking those in the amber repo?

@faustinoaq
Copy link
Contributor Author

Since .min.js and .bundle.js files are essentially compiled, why are we tracking those in the amber repo?

You're right this is just an update of NPM/JS assets (to avoid vulnerability alerts on GH). I will remove them in my next PR about new amber watch configuration 😉

For now I think this is ok 👍

@drujensen
Copy link
Member

@robacarp @faustinoaq we leave the compiled assets so that the site will display immediately without requiring npm to finish compiling. Any reason to remove them?

@faustinoaq
Copy link
Contributor Author

faustinoaq commented May 31, 2018

we leave the compiled assets so that the site will display immediately without requiring npm to finish compiling.

@drujensen Yeah, that's a good reason. In my next PR (new amber watch config). I will use a isolated vanilla.js script (Optional Webpack/NPM) for amber and hot reloading. so this won't be a problem anymore.

For now, I think this is ok 👍

@robacarp
Copy link
Member

@drujensen I’m sorry, I forgot we’ve discussed this before at least once. I’m fine with that. My compulsion is to never commit compiled or transpiled code...but I remember the reason behind this now.

@robacarp robacarp merged commit e779177 into master Jun 13, 2018
@robacarp robacarp deleted the fa/update-assets branch June 13, 2018 18:58
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.

4 participants