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

chore(v2): upgrade dependencies + require Node 12 #4223

Merged
merged 14 commits into from
Feb 18, 2021
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 14, 2021

Motivation

Follow-up of #4148

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview/E2E/Local dev.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 requested a review from slorber as a code owner February 14, 2021 22:19
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 14, 2021
@netlify
Copy link

netlify bot commented Feb 14, 2021

Deploy preview for docusaurus-2 ready!

Built with commit dae33ca

https://deploy-preview-4223--docusaurus-2.netlify.app

@netlify
Copy link

netlify bot commented Feb 14, 2021

[V1] Deploy preview success

Built with commit dae33ca

https://deploy-preview-4223--docusaurus-1.netlify.app

"postinstall": "yarn lock:update && yarn build:packages",
"postinstall": "run-p postinstall:**",
"postinstall:main": "yarn lock:update && yarn build:packages",
"postinstall:dev": "is-ci || husky install",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgrade Husky (see https://dev.to/typicode/what-s-new-in-husky-5-32g5 for more details)

@@ -5,35 +5,41 @@
* LICENSE file in the root directory of this source tree.
*/

import sitemap, {Sitemap, SitemapItemOptions} from 'sitemap';
import {SitemapStream, streamToPromise} from 'sitemap';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/ekalinin/sitemap.js/blob/master/CHANGELOG.md#500

This release is heavily focused on converting the core methods of this library to use streams. Why? Overall its made the API ~20% faster and uses only 10% or less of the memory.

@@ -16,7 +17,7 @@ export const DEFAULT_OPTIONS: Required<PluginOptions> = {
};

export const PluginOptionSchema = Joi.object({
cacheTime: Joi.number().positive().default(DEFAULT_OPTIONS.cacheTime),
cacheTime: Joi.number().positive().default(DEFAULT_OPTIONS.cacheTime), // deprecated
Copy link
Contributor Author

@lex111 lex111 Feb 14, 2021

Choose a reason for hiding this comment

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

How do we manage deprecated fields? Should we notify the user about this (via console.log)?

cacheTime is being dropped from createSitemapIndex - This didn't actually cache the way it was written so this should be a non-breaking change in effect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have some examples in classic theme:

  disableDarkMode: Joi.any().forbidden(false).messages({
    'any.unknown':
      'disableDarkMode theme config is deprecated. Please use the new colorMode attribute. You likely want: config.themeConfig.colorMode.disableSwitch = true',
  }),

(not sure why there's a "false", looks like a typo btw)

@@ -210,12 +210,12 @@ function extractSourceCodeAstTranslations(
path.node.openingElement.name.name === 'Translate'
) {
// We only handle the optimistic case where we have a single non-empty content
const singleChildren: NodePath | undefined = path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After bump of @babel/core, apparently type NodePath has been "extended", so I removed its using in two places so that the build succeeds again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks fine to me.

I think I added the types because TS complained otherwise so if it still compiles... ;)

@github-actions
Copy link

github-actions bot commented Feb 14, 2021

Size Change: +8 B (0%)

Total Size: 158 kB

ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 17.6 kB 0 B
website/build/assets/js/main.********.js 111 kB +9 B (0%)
website/build/blog/2017/12/14/introducing-docusaurus/index.html 21.7 kB -1 B (0%)
website/build/docs/introduction/index.html 180 B 0 B
website/build/index.html 6.95 kB 0 B

compressed-size-action

@lex111
Copy link
Contributor Author

lex111 commented Feb 14, 2021

1:20:23 AM: error human-signals@2.1.0: The engine "node" is incompatible with this module. Expected version ">=10.17.0".

Hmm, one of the execa v5 dependencies requires node >=10.17.0, maybe we should switch to this version as minimum node version for our packages (given the EOL of 10.x ends in April https://github.com/nodejs/Release#release-schedule)?

@@ -40,7 +40,7 @@ describe('lastUpdate', () => {
expect(consoleMock).toHaveBeenCalledTimes(1);
expect(consoleMock).toHaveBeenCalledWith(
new Error(
`Command failed with exit code 128: git log -1 --format=%ct, %an ${nonExistingFilePath}`,
`Command failed with exit code 128: git log -1 --format=%ct, %an ${nonExistingFilePath}\nfatal: ambiguous argument '${nonExistingFilePath}': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'`,
Copy link
Collaborator

@slorber slorber Feb 15, 2021

Choose a reason for hiding this comment

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

any reason for this change?

The Windows CI does not seem too happy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To pass the tests, apparently something has changed in execa v5 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what's the easiest way to do that with Jest, but I think just matching the error message beginning should be fine.

Done this in another place:

    expect(consoleSpy.mock.calls[0][0].message).toMatch(/^Command failed with exit code 128/);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it a little differently, the test passed, but Windows CI still failed.

@slorber
Copy link
Collaborator

slorber commented Feb 15, 2021

Hmm, one of the execa v5 dependencies requires node >=10.17.0, maybe we should switch to this version as minimum node version for our packages (given the EOL of 10.x ends in April https://github.com/nodejs/Release#release-schedule)?

Yes, I think it's a good time to move to Node 12 min

@github-actions
Copy link

github-actions bot commented Feb 15, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 78
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4223--docusaurus-2.netlify.app/classic/

@slorber slorber changed the title chore(v2): upgrade dependencies chore(v2): upgrade dependencies + require Node 12 Feb 16, 2021
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Feb 16, 2021
@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2021

A strange bug on WIndows CI - all tests pass, but exit code 1 is returned (see jestjs/jest#9324).

Test Suites: 68 passed, 68 total
Tests:       499 passed, 499 total
Snapshots:   254 passed, 254 total

@slorber
Copy link
Collaborator

slorber commented Feb 17, 2021

this is weird, don't know why.
Maybe it's Jest 26 behavior?

We have this line in logs.

ReferenceError: You are trying to import a file after the Jest environment has been torn down.

Also someone reported this here: jestjs/jest#9324 (comment)

Maybe worth trying to fix this error and see?

@slorber
Copy link
Collaborator

slorber commented Feb 17, 2021

Giving it a try. I think it's related to bad async tests in Jest that leak resources.

yarn test packages/lqip-loader/src/__tests__/index.test.ts --detectOpenHandles

Running Jest with --detectOpenHandles should not produce errors

@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2021

@slorber awesome, CI passed, thanks!

@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2021

Do we need to install the new version of immer via resolutions field to fix #4191? When react-dev-utils updates this package, we can remove this resolution. WDYT?

@slorber
Copy link
Collaborator

slorber commented Feb 17, 2021

I don't think resolutions applies to dependencies, only our own repo, and it's yarn only so it's unlikely to solve any problem imho

@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2021

Alright, so I think the PR is ready for a merge.

@slorber
Copy link
Collaborator

slorber commented Feb 18, 2021

thanks, LGTM

@slorber slorber merged commit f13448d into master Feb 18, 2021
@lex111 lex111 deleted the lex111/deps1302 branch February 19, 2021 18:20
@lex111 lex111 added this to the v2.0.0-alpha.71 milestone Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: breaking change Existing sites may not build successfully in the new version. Description contains more details.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants