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

Travis: Fail when changed files are detected #1819

Merged
merged 11 commits into from
Mar 24, 2019

Conversation

RunDevelopment
Copy link
Member

This resolves #1785.


I'll make some testing on this PR, so there might be some failed tests here and there. I'll comment again when I'm ready.

@RunDevelopment
Copy link
Member Author

Alright, it failed when some generated files were changed and it succeeds when Prism is built properly.

Seems to work.

@mAAdhaTTah
Copy link
Member

Love the idea. Two nits:

  1. In the gulpfile.js, we tend to define the function separately from the exports, so we should do the same here. I'm specifically thinking this will be useful once we land chore(package): Add ESLint #1796, we can add eslint + this check to like a "premerge" task. Which leads to...
  2. ...maybe we should change the task name to premerge or something of that nature? Bikeshedding, but I don't love that name. I might suggest needsRebuild as the function name, and premerge as the task name, so we can expand premerge later, but I'm open to other ideas.

@RunDevelopment
Copy link
Member Author

Alright, funny story:

The build fails because some minified files changed. I had to make a fresh npm installation until these files also changed on my system.
I don't know the concrete behavior of npm install but it doesn't seem to update dependencies when the new release is only a patch. A patch which is uglify's case changed the minification behavior.

So yeah. With the current state of this PR, the success of our builds will be at the mercy of Uglify's minification behavior.

Maybe it's time for a package-lock.json?

@mAAdhaTTah
Copy link
Member

Yeah, and we'll need to update the install step to use npm ci. That can go in this PR, since both are dealing with Travis.

@mAAdhaTTah mAAdhaTTah added this to the 1.16.0 milestone Mar 24, 2019
@RunDevelopment
Copy link
Member Author

Minor problem: The npm version of Node v6 doesn't support npm ci. Any ideas?

@mAAdhaTTah
Copy link
Member

Upgrade npm in travis?

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Mar 24, 2019

Updating to the latest npm version works but it feels kind of hacky...

Well, anyway. @mAAdhaTTah, you might want to take another look at this PR.

@mAAdhaTTah
Copy link
Member

This works for me. Since we're only updating to use npm ci, which relies solely on the package-lock.json, I don't think we're likely to run into issues if we stick with the latest version.

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.

Travis: Fail if files change after gulp
2 participants