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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include package-lock.json when building dist #29

Merged
merged 3 commits into from
Feb 20, 2018

Conversation

masterful
Copy link
Contributor

馃 this boilerplate has been super helpful, thank you!

Would it make sense to include the package-lock.json file as part of the build step? Otherwise the versions used while testing/developing could change slightly during the build step, right?

@irvinlim
Copy link
Owner

irvinlim commented Feb 20, 2018

Hi @masterful, glad to know that it had helped you!

Good point - was an oversight on my part. I think what you say makes sense - since after all ensuring that environments are kept constant is part of CI/CD.

Additionally, we should also change the command to npm install --from-lock-file, since a normal npm install would overwrite the lockfile, right?


Also, seems like the Travis build can't pass since forks do not have access to the encrypted environment variables, which includes AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY.

I've changed some of the Travis logic so that npm run test is only run when these encrypted env vars are available. I've merged master into your branch so that the build will pass.

@masterful
Copy link
Contributor Author

馃 I'm not seeing information about that flag on the docs, and hadn't heard of it before
It was my understanding that, if not present, a file would be generated, but otherwise it would use the existing file rather than replacing it...

From the sounds of it, the flag you're referring to has a slightly different use case in mind?
npm/npm#18286

But yeah, there's a comment on that thread saying that it should be reproducible builds without that flag.

@irvinlim
Copy link
Owner

irvinlim commented Feb 20, 2018

Hmm... I just gave that GitHub issue thread a read again, and it seems that somehow I've always thought that the flag had all along existed.

Looks like you're right - on the latest versions of NPM (^5.4.2), the installation of dependencies depends on the already-present package-lock.json file.

I'll be merging this in then! :shipit: Thank you for your contribution and help! 馃槃

@irvinlim irvinlim merged commit f46dc98 into irvinlim:master Feb 20, 2018
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.

None yet

2 participants