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

[MAJOR] strip-bom@4 update #71

Merged
merged 1 commit into from
Nov 7, 2019
Merged

[MAJOR] strip-bom@4 update #71

merged 1 commit into from
Nov 7, 2019

Conversation

brodybits
Copy link
Contributor

@brodybits brodybits commented Jun 11, 2019

Platforms affected

All

Motivation and Context

Updated dependencies are generally preferred, in order to reduce the chance of missing security updates in the future.

This PR is raised after checking the results of npm outdated, as described in https://github.com/apache/cordova-coho/blob/master/docs/tools-release-process.md#update-and-pin-dependencies.

If approved, I think this change should be included in an upcoming minor or major release. I do not think this upodate should block the release with fs-extra@8 update (PR #70).

Description

Update to use strip-bom version 4 (^4.0.0) in dependencies

Testing

  • npm test passes on the supported Node.js versions 6, 8, and 10
  • Verified that AppVeyor CI passes
  • Verified that Travis CI passes

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Jun 11, 2019
@raphinesse
Copy link
Contributor

Requires node >= 8. Meaning we cannot update this before the next major.

@brodybits brodybits changed the title strip-bom@4 update [MAJOR] strip-bom@4 update Jun 13, 2019
@brodybits
Copy link
Contributor Author

brodybits commented Jun 13, 2019

I just marked this proposal as MAJOR in the title. While strip-bom@4.0.0 does not seem to cause a failure on Node.js 6, that engines line means this can change in another patch or minor release of strip-bom. Thanks to @raphinesse for spotting this pitfall.

A couple more questions:

Is there anything else I should do besides marking this as MAJOR in the title to avoid a premature merge?

How should we decide whether or not to allow breaking changes in master?

Is there anything else we should do to avoid breaking changes at random in master?

I ask the second question since breaking changes seemed to happen almost at random in the past. While we can always use branching to support patch releases, I think we should avoid this whenever practical.

@dpogue dpogue added this to the 4.0.0 milestone Oct 30, 2019
Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Nov 7, 2019
@erisu erisu merged commit 6583aef into apache:master Nov 7, 2019
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Development

Successfully merging this pull request may close these issues.

None yet

4 participants