search

ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 208 forks source link

Bump ansi-regex from 3.0.0 to 3.0.1 #362

Open dependabot[bot] opened 1 year ago

dependabot[bot] commented 1 year ago

Bumps ansi-regex from 3.0.0 to 3.0.1.

Commits


Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/ToothlessGear/node-gcm/network/alerts).
mtrezza commented 1 year ago

@ToothlessGear This needs to be rebased on master, but I don't see the button. Could you do that, or elevate my permissions then I'll do it?

ToothlessGear commented 1 year ago

@mtrezza I can merge it sure, but you are a full collaborator. Rebase merging is enabled, can you recheck?

mtrezza commented 1 year ago

The thing is that this PR doesn't seem to be based on the latest commit on master. The CI doesn't show the new job titles but the old ones. Usually I'd see a button to rebase and merging should be blocked. But in this case either I don't have the permissions to rebase, or the button is not displayed because the rebase isn't required to merge, which IMO should be set as required in the repo settings. If we allow to merge a PR without rebase, the CI tests are less effective and may fail only after merging.

ToothlessGear commented 1 year ago

@dependabot rebase

ToothlessGear commented 1 year ago

@mtrezza I've added the latest update requirements for npm, snyk. For node it's only 16 of the matrix since it's LTS.

mtrezza commented 1 year ago

Yes, but I guess we are still officially supporting Node 12, even though it's out of LTS. If we decide to stop supporting Node 12, we'd have to do a major version increment because it's a breaking change, to follow semver. As for now, we still need to test for Node 12 until we announce that breaking change.

In fact, officially we are still supporting Node >=4: https://github.com/ToothlessGear/node-gcm/blob/e034e7ddef82f47e91a32adb868e1436a3218d5a/package.json#L83-L85

To clean this up, I think we could:

  1. leave all the Node tests mandatory for now (regardless of their LTS support, I bet there are still deployments that run on even older node versions)
  2. merge the most important existing PRs (any bug fixes, minor improvements, etc)
  3. publish a release (to be graceful to users of Node <16)
  4. make the cut: bump the engines.node in package.json and remove obsolete CI tests to support only from latest LTS node version and publish with a major version increment

Regardless, the original issue I mentioned seems to be solved now, I see the update button in another PR:

image
mtrezza commented 1 year ago

I see you've also disabled all merge options except for "Rebase and merge", I wouldn't do that. I would only enable "Squash and merge" and disable all others. We'll also need that for release-automation. A rebase as part of a merge is rather unpredictable. This btw. has nothing to do with requiring a PR to be up-to-date with the base branch, which is a separate setting that you have already set.

ToothlessGear commented 1 year ago 
1. leave all the Node tests mandatory for now (regardless of their LTS support, I bet there are still deployments that run on even older node versions)

Done.

I would only enable "Squash and merge" and disable all others.

I'd rather keep all of them. If the commits of the PR are nice to follow, well described and atomic, I'd rather rebase than squash. You have the choice when merging.

mtrezza commented 1 year ago

Once we implement release automation we can only allow "squash and merge", other merge types create a commit history that is not parsable for that purpose.

A "rebase and merge" (which is still enabled) doesn't make much sense in our workflow IMO, because a rebase always needs to happen before we approve a PR as the CI needs to run on it. Otherwise the CI runs tests on something that will not be the end result (pre-rebase) of that "rebase and merge" operation.