-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs] Use custom webpack loader for markdown #26774
Conversation
As a side note, we broke HMR support for the markdown of the page a while ago. I don't think that this change will help or harm this aspect, I'm only raising it as it's may be relevant in the future. Is reading the markdown on its editor good enough? I don't think so, nor that it's close to good enough. HMR of the markdown allows to work on the actual content developers will see when consuming the content. Losing sight of this is a good way to write markdown that looks OK on its editor but KO once rendered (e.g. title too long to fit, wrong code indentation, overwhelming code sections, etc.). I personally frequently reload the page (it's slower), to make sure it's OK, as I work on the content. It would be interesting to see if other contributors have the same frustration. |
Yep that's a problem. But it improves user experience by a lot which is more important. For live reloading of markdown you could always use the preview in your code editor (vscode has this built-int). |
30451ee
to
3f9b279
Compare
Makes more sense to move rendering markdown to the loader in this PR as well. Some pages don't follow the pages/somePage <-> src/pages/somePage mapping. The follow-up would be just as big and these concerns are tightly coupled anyway. |
ef7625f
to
e94dc96
Compare
The rest is loaded on demand. The "directory -> raw files" assumption breaks down on blog posts. These posts are all in the same directory.
e94dc96
to
051fad5
Compare
Pushed "render markdown at bundle time" to make it clearer that hot reloading is going to be improved. Changes to |
5f91397
to
1a6775e
Compare
2786bb8
to
57775f8
Compare
Has been open for a while. Will merge after the updating the branch and then we'll see if we discover any problems. |
// FIXME: Revert before merging | ||
if (process.env.PULL_REQUEST === 'false' && !l10nPRInNetlify && !vercelDeploy) { |
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'm taking care of it #26970.
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.
@eps1lon It looks pretty cool! 💯 for the HMR of the markdown. I hold on to the release because I wanted to try dynamic pages with Next.js, and see how this change fits into the equation, but I didn't get the time, so it's fine.
Why do I mean by dynamic page? It seems inefficient to build each API page (>100) when it always renders the same type of content. So my thought was: why don't we create one dynamic /api page. This way, we can build it once, saving x minutes (to be measured) of builds and dynamically render the .json of the different components. We didn't touch the API pages, so 🙆♂️
The changes seem to have broken the docs in development on Windows: #21053 (comment). |
Problem: Previous usage of
require-context
with inline loaders is no longer supported in webpack 5 (webpack/webpack#12900)Solution: Replace with a single import + custom loader that fakes require.context
Preview: https://deploy-preview-26774--material-ui.netlify.app/components/tables/
Ignore 1a6775e since this is just the change from using
npx @codemod/cli docs/pages --remote-plugin https://astexplorer.net/#/gist/5a4cf1d9e15657695ae270f5c7819b2c/2e489edc6b0014506602f34c069d6428b448e1e5
(https://astexplorer.net/#/gist/5a4cf1d9e15657695ae270f5c7819b2c/2e489edc6b0014506602f34c069d6428b448e1e5)Additional benefits:
ComponentLinkHeader
is always imported in MarkdownDocs even though it might now be used. The code is not tree-shakeable at the moment)Isn't this tying us more to webpack?
Just as much as we were tied before since require.context is webpack specific.
TODO:
require.context('!raw-loader!')
usagesFollow-Up: