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.

Sign up for GitHub

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

Jump to bottom

Add mdn- and Node.js 8 deprecation warnings for 1.1 release #6777

Merged
merged 10 commits into from
Oct 2, 2020
Merged

Add mdn- and Node.js 8 deprecation warnings for 1.1 release #6777

merged 10 commits into from
Oct 2, 2020

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Sep 30, 2020

Another entry in the #6640 saga and the v1.1 counterpart to the v2.0.0 changes in #6713.

What this PR attempts to do

  • Adds a notice to the top of the README (which will appear on the npm package page)
  • Adds docs for upgrading 1.1.x → 2.0.x
  • Adds a deprecation notice that the package name is changing and to use the scoped package instead. It links to the aforementioned upgrade doc
  • Adds a deprecation notice for Node.js 8. This should only fire if Node.js 8 is detected in the process.version string

What needs a close look

  • Do the deprecation warnings make sense? Are they too wordy or not wordy enough?
  • Do the upgrade instructions make sense? Are there any missing steps?
  • Would you be comfortable seeing this ship in the next release of BCD?

Additional notes

  • I've tested the deprecation notices on my computer (using nodenv to switch between Node.js versions), but I'd welcome tests from others. You can install it from my branch directly: npm install https://github.com/ddbeck/browser-compat-data#add-deprecation-warnings
  • The link to the UPGRADE doc from the deprecation notice won't work until there's a tag for v1.1.0. This is expected; the tag will be created by npm version when I do the 1.1 release
  • The pattern for emitting the warnings once and only once comes from Node.js's process.emitWarning() docs. It's a bit verbose, but it seems to be preferred

Known follow-up tasks

Once the master-scoped-package branch merges back into master, we'll want to:

  • Make sure the notice at the top of the README goes away
  • Remove the upgrade doc (I assume linking the file with the tag ref will be adequate)

@ddbeck
Add deprecation notices
03cf0ff 
On Node.js 8, you'll get messages like this:

```
(node:56410) DeprecationWarning: mdn-browser-compat-data is deprecated. Upgrade to @mdn/browser-compat-data: (TODO: upgrade doc URL)
(node:56410) DeprecationWarning: mdn-browser-compat-data: @mdn/browser-compat-data ends support for Node.js 8. Upgrade to Node.js 10 or later.
```

On Node.js 10, you'll get a message like this:

```
(node:56752) DeprecationWarning: mdn-browser-compat-data is deprecated. Upgrade to @mdn/browser-compat-data: (TODO: upgrade doc URL)
```
@ddbeck
Add upgrade docs
85ec86a
@ddbeck
Add upgrade docs URL to deprecation notice
0276e80
@ddbeck
Add upgrade docs link to README
8677b4d
@ddbeck
Copyedit upgrade docs
76bdf33
@ddbeck
Use full semver version in UPGRADE doc
186abfb
@ddbeck ddbeck requested a review from Elchi3 as a code owner September 30, 2020 15:27
@ddbeck ddbeck removed the request for review from Elchi3 September 30, 2020 15:28
@github-actions github-actions bot added the docs Issues or pull requests regarding the documentation of this project. label Sep 30, 2020
@ddbeck
Copy link
Collaborator Author

ddbeck commented Sep 30, 2020

Oh good, we got the correct deprecation notices in tests:

@ddbeck
Copy link
Collaborator Author

ddbeck commented Sep 30, 2020

Reviewing the diff here, I noticed that the Node version-checking code runs whether the warning has been emitted already or not. I moved it into the warning function and confirmed that the tests still show the correct warnings.

chrisdavidmills
chrisdavidmills requested changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small bits, not nothing major. Nice work @ddbeck !

README.md Outdated
@@ -1,3 +1,5 @@
**Warning:** This package is soon-to-be renamed. Use [`@mdn/browser-compat-data`](https://www.npmjs.com/package/@mdn/browser-compat-data) instead. If you're already using `mdn-browser-compat-data`, read [the upgrade guide](https://github.com/mdn/browser-compat-data/blob/v1.1.0/UPGRADE-2.0.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soon-to-be doesn't need hyphens in this context, imo. If it was in a context like "The soon-to-be-renamed browser-compat-data package", then it would have hyphens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, fixed with 638d3fc. I wasn't sure if I was modifying is or renamed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And fixed again with 2aba539.

UPGRADE-2.0.x.md Outdated

If possible, run your test suite to make sure this worked.

If you encountered any undocumented breaking changes as a result of this upgrade, please [open an issue](https://github.com/mdn/browser-compat-data/issues/new).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link "breaking changes" to the section named "Before you start", above, as this is where the breaking changes are documented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Fixed with 11551cd.

chrisdavidmills
chrisdavidmills approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nice one @ddbeck .

@ddbeck ddbeck merged commit ee7feac into mdn:master Oct 2, 2020
@ddbeck ddbeck deleted the add-deprecation-warnings branch October 2, 2020 14:15
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Oct 8, 2020
@ddbeck
Revert "Add mdn- and Node.js 8 deprecation warnings for 1.1 release (m…
f19ba34 
…dn#6777)"

This applies only to the 1.1.x releases.

This reverts commit ee7feac.
ddbeck added a commit that referenced this pull request Oct 29, 2020
@ddbeck
Merge master-scoped-package to master (#7155)
b816c3a 
This concludes the `mdn-` package name and continues development under the `@mdn/browser-compat-data` name.

* Implement 2.0.0 breaking changes (#6713)

* Rename package and increment version to pre-2.0
* Require Node 10 or later
* Rename package in the docs
* Increment prerelease version

* Initial release of @mdn/browser-compat-data

A continuation of mdn/browser-compat-data

* Patch release containing data or non-breaking updates only

* Revert "Add mdn- and Node.js 8 deprecation warnings for 1.1 release (#6777)"

This applies only to the 1.1.x releases.

This reverts commit ee7feac.

* Patch release containing data or non-breaking updates only

* Patch release containing data or non-breaking updates only

* Use Node.js ≥10 for CI and drop Travis (#6797)

* Use Node.js ≥10 for CI
* Remove Travis CI config
* Remove badges from README

* Patch release containing data or non-breaking updates only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdavidmills chrisdavidmills chrisdavidmills approved these changes

Assignees
No one assigned
Labels
docs Issues or pull requests regarding the documentation of this project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ddbeck @chrisdavidmills