[core] Remove outdated docsearch.js dependency #6242
Conversation
|
These are the results for the performance tests:
|
|
It seems like it has removed some used dependencies as a side effect |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
force-pushed
the
remove-dead-docsearch-dependency
branch
from
d2c7e83
to
036f631
Compare
github-actions
bot
removed
the
PR: out-of-date
|
Wow, so strange, why would remove npm dependencies changes how webpack match aliases? As far as I could push the exploration, the issue is that const requireDocs = require.context('docsx/data', true, /js$/);Gets converted by webpack alias. As far as I know https://github.com/tleunen/babel-plugin-module-resolver isn't reading const requireDocs = require.context('./docs/data', true, /js$/);Which is then turned by the second alias to: const requireDocs = require.context('./node_modules/@mui/monorepo/docs/data', true, /js$/);Where it fails. The fix I'm proposing is to have webpack also try to resolve with: const requireDocs = require.context('./docs/data', true, /js$/); |
oliviertassinari
force-pushed
the
remove-dead-docsearch-dependency
branch
from
2e7dc7b
to
9dba018
Compare
oliviertassinari
changed the title
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
|
It would be nice to push this PR forward as it will resolve a critical dependabot alert |
|
The webpack fail is indeed weird |
oliviertassinari
force-pushed
the
remove-dead-docsearch-dependency
branch
2 times, most recently
from
f8509b3
to
ea17607
Compare
github-actions
bot
removed
the
PR: out-of-date
oliviertassinari
force-pushed
the
remove-dead-docsearch-dependency
branch
3 times, most recently
from
c6a1a76
to
f6df5e7
Compare
| // TODO: 'browserslist:modern' | ||
| target: 'web', |
There was a problem hiding this comment.
Copy the mono repo
| ) => {...} | ||
|
|
||
| // Imperative subscription | ||
| ) => {...} |
There was a problem hiding this comment.
Something I found by chance, it's not ideal to have these trailing spaces when you copy & paste https://next.mui.com/x/react-data-grid/events/#catalog-of-events in your IDE:
There was a problem hiding this comment.
I think that we could enforce a rule in the CI with mui/material-ui#34686 (comment) to have the PRs fail if they have trailing spaces.
| @@ -177,9 +179,7 @@ | |||
| "**/@mui/monorepo" | |||
| ] | |||
| }, | |||
| "dependencies": { | |||
| "html-webpack-plugin": "^5.5.0" | |||
There was a problem hiding this comment.
Wrong location, this is a dev dependency.
| @@ -149,6 +150,7 @@ | |||
| "nyc": "^15.1.0", | |||
| "playwright": "^1.21.1", | |||
| "prettier": "^2.7.1", | |||
| "process": "^0.11.10", | |||
There was a problem hiding this comment.
Required as we were relying on it, with a transitive dependency of docsearch.js.
| ...webpackBaseConfig.resolve, | ||
| alias: { | ||
| ...webpackBaseConfig.resolve.alias, | ||
| docs: false, // Disable this alias as it creates a circual resolution loop with the docsx alias |
There was a problem hiding this comment.
Since we are not supposed to import from the docs-infra or mono-repo docs in the visual regression tests, this works as well. It's more isolated than the first attempt:
- docs: path.resolve(__dirname, './node_modules/@mui/monorepo/docs'),
+ docs: [
+ path.resolve(__dirname, './node_modules/@mui/monorepo/docs'),
+ path.resolve(__dirname, './docs'),
+ ],There was a problem hiding this comment.
Actually, I might not have needed to fix this, I see that it's already all broken on HEAD, e.g. https://app.circleci.com/pipelines/github/mui/mui-x/25566/workflows/5b4b75f8-3d75-4d0f-8c6e-90812dd0563a/jobs/147039
|
I have updated the approach, see if you are happy with it. |
| // TODO: Why does webpack include a key for the absolute and relative path? | ||
| // We just want the relative path | ||
| if (!path.startsWith('./')) { | ||
| return res; | ||
| } | ||
|
|
There was a problem hiding this comment.
I have copied mui/material-ui#28248.
| ...webpackBaseConfig.resolve, | ||
| alias: { | ||
| ...webpackBaseConfig.resolve.alias, | ||
| docs: false, // Disable this alias as it creates a circual resolution loop with the docsx alias |
There was a problem hiding this comment.
Actually, I might not have needed to fix this, I see that it's already all broken on HEAD, e.g. https://app.circleci.com/pipelines/github/mui/mui-x/25566/workflows/5b4b75f8-3d75-4d0f-8c6e-90812dd0563a/jobs/147039
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
force-pushed
the
remove-dead-docsearch-dependency
branch
from
ed6c0d7
to
079371b
Compare
github-actions
bot
removed
the
PR: out-of-date
|
The impact of the fix is now a lot more scoped than in the first attempt, I have made the configuration closer to the one used in https://github.com/mui/material-ui/. Since it's not really important to get peer feedback on this, and it has been pending for a week or two, I'm moving forward 😁 |
In the future, we should look into how we can have the docs-infra control it's own npm dependencies so mui/material-ui#34421 is enough.