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.
Upgrade Husky (see https://dev.to/typicode/what-s-new-in-husky-5-32g5 for more details)
| */ | ||
|
|
||
| import sitemap, {Sitemap, SitemapItemOptions} from 'sitemap'; | ||
| import {SitemapStream, streamToPromise} from 'sitemap'; |
There was a problem hiding this comment.
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.
|
|
||
| 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.
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.
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)
| 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.
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.
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)? |
| 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.
any reason for this change?
The Windows CI does not seem too happy
There was a problem hiding this comment.
To pass the tests, apparently something has changed in execa v5 🤷♂️
There was a problem hiding this comment.
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.
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.)