-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Conversation
Deploy preview for docusaurus-2 ready! Built with commit dae33ca |
[V1] Deploy preview success Built with commit dae33ca |
"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", |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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... ;)
Size Change: +8 B (0%) Total Size: 158 kB ℹ️ View Unchanged
|
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>...]'`, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤷♂️
There was a problem hiding this comment.
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/);
There was a problem hiding this comment.
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.
Yes, I think it's a good time to move to Node 12 min |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4223--docusaurus-2.netlify.app/classic/ |
A strange bug on WIndows CI - all tests pass, but exit code 1 is returned (see jestjs/jest#9324).
|
this is weird, don't know why. We have this line in logs.
Also someone reported this here: jestjs/jest#9324 (comment) Maybe worth trying to fix this error and see? |
Giving it a try. I think it's related to bad async tests in Jest that leak resources.
Running Jest with |
@slorber awesome, CI passed, thanks! |
Do we need to install the new version of immer via |
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 |
Alright, so I think the PR is ready for a merge. |
thanks, LGTM |
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.)