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

feat: migrate webpack file-loader to asset modules #4708

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Apr 30, 2021

Breaking changes

  • require("./img/image.png").default becomes require("./img/image.png")

Motivation

url-loader and file-loader are going to be deprecated in favor of asset modules

This is an attempt to migrate to asset modules.

Some initial problems encountered:

cc @alexander-akait

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

preview

Related PRs

Follow-up of #4089

@slorber slorber requested a review from lex111 as a code owner April 30, 2021 17:06
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 30, 2021
@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Apr 30, 2021
@netlify
Copy link

netlify bot commented Apr 30, 2021

[V1]

Built with commit 645bf67

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

node.url
? `src={require("${inlineMarkdownImageFileLoader}${pathUrl}").default}`
: ''
node.url ? `src={require("${pathUrl}?${assetQuery}")}` : ''

Choose a reason for hiding this comment

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

I recommend to avoid this stuff, you can do src={new URL('./image.png', import.meta.url)}, it is more native for ES modules and in future we should prefer to this way to get asset URL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, replaced and it seems to work as before

Is it planned to support some kind of ESM-based DX? (I mean similar to Snowpack?)

Is it the recommended way to link to a file now?

<a
  target="_blank"
  href={new URL(
    '../../assets/docusaurus-asset-example-pdf.pdf',
    import.meta.url,
  ).toString()}>
  Download this PDF
</a>

Choose a reason for hiding this comment

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

Is it planned to support some kind of ESM-based DX? (I mean similar to Snowpack?)

We thought about it and measured it, but at the moment memory cache (default in development mode) is faster and more stable, try to get snowpack and build big project and watch it, most likely you won't even be able to run it, with 5k modules my browser just freeze, with 10k snowpack fails.

In only JS world it is easier to implement ESM in browser, because you need parse import/export tokens and build graph and watch them, but when you get ts/css/assets/less/sass/pofyfills/etc and etc it is not easy, you need to write tokenizer for each source and regexps is not safe solution, other problems source maps (here long story).

There is also a problem that dev and prod will be different, it can lead to unexpected problems too.

And here a lot of limitations.

I don't want to say it is impossible, I want to say we need some more time for this.

Is it the recommended way to link to a file now?

Yes, we don't have import file from './text.pdf' in spec, so it is recommend way, maybe import assert will change something in future https://github.com/tc39/proposal-import-assertions

function fontAssetRule(): RuleSetRule {
return {
...baseAssetRule('fonts'),
test: /\.(woff|woff2|eot|ttf|otf)$/,

Choose a reason for hiding this comment

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

Small note - extensions can be woff, WOFF, so I prefer to use i flag for all regexp, but this not related to this PR 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks :)

# Conflicts:
#	packages/docusaurus-mdx-loader/src/remark/transformImage/index.js
#	packages/docusaurus-mdx-loader/src/remark/transformLinks/index.js
@@ -108,6 +108,7 @@ export function createBaseConfig(
chunkFilename: isProd
? 'assets/js/[name].[contenthash:8].js'
: '[name].js',
assetModuleFilename: 'assets/[name]-[hash][ext]',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assetModuleFilename: 'assets/[name]-[hash][ext]',
assetModuleFilename: 'assets/[name]-[contenthash][ext]',

Usually contenthash gives better caching results.

Choose a reason for hiding this comment

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

Yep, prefer to use contenthash

@Josh-Cena Josh-Cena linked an issue Dec 4, 2021 that may be closed by this pull request
@slorber slorber removed this from the 2.0.0 milestone Dec 16, 2021
@slorber
Copy link
Collaborator Author

slorber commented Jan 20, 2022

Note for self, upgrade to css-loader 6 as part of this PR (see also #6424)

@Josh-Cena Josh-Cena changed the title chore(v2): migrate webpack file-loader to asset modules feat: migrate webpack file-loader to asset modules May 25, 2022
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: maintenance This PR does not produce any behavior differences to end users when upgrading. status: blocked This issue is blocked by another issue or external dep and can't be pushed further.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[webpack 5] Yarn v2 can't resolve file loader in docs plugin
5 participants