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

Support /** webpackIgnore: true */ #1256

Closed
slorber opened this issue Feb 5, 2021 · 14 comments · Fixed by #1264
Closed

Support /** webpackIgnore: true */ #1256

slorber opened this issue Feb 5, 2021 · 14 comments · Fixed by #1264

Comments

@slorber
Copy link

slorber commented Feb 5, 2021

  • Operating System: MacOs
  • Node Version: 14
  • NPM Version: 6
  • webpack Version: 4.44.1
  • css-loader Version: 5.0.1

Feature Proposal

Allow urls to not be converted to require() calls, on a per-url basis.

url('noRequire:/fonts/font.ttf') should just be converted to url('/fonts/font.ttf') without any processing

(noRequire is maybe not a good name but you understand the idea).

I wasn't able to find in a doc to escape on a per-url basis from this url->require conversion behavior

Feature Use Case

I'm the maintainer of https://github.com/facebook/docusaurus and we are doing preparatory work to upgrade to Webpack 5, that will impact thousands of doc sites.

We used css-loader@3, and some users had url("/fonts/font.eot"), and it did not get processed before.

After upgrading to css-loader@5 (PR: facebook/docusaurus#4081)
for some reason an URL that worked before now fails.

url("/fonts/font.eot") used to work, but does not anymore, as it's converted to a require call.

url("!!file-loader!/static/fonts/font.eot") is a good workaround, but I don't feel like we should recommend that to our large user-base, that are not even all frontend developers (we are a documentation tool)

I don't particularly want to disable fully this URL => require() conversion, as I think it's better to let webpack process/hash assets for caching purpose, just having a convenient escape hatch to fallback to no processing at all.

Does it make sense?

Note: it's not convenient for our users to customize the webpack config with site-specific rules

@alexander-akait
Copy link
Member

alexander-akait commented Feb 6, 2021

What is use case? webpack allows to resolve /fonts/font.eot, you just need setup resolve.roots (by default is is context).

The main answer why it should be not resolved?

url('noRequire:/fonts/font.ttf')

Sorry but it is broken CSS syntax and will be bad idea.

I agree with:

/* webpackIgnore: true */
url('/fonts/font.ttf')

@slorber
Copy link
Author

slorber commented Feb 8, 2021

What is use case?

Retro compatibility.
It used to not be resolved before this upgrade (https://github.com/facebook/docusaurus/pull/4081/files) and now it is.

The main answer why it should be not resolved?

To keep the behavior as it was before.
We are an opiniated mainstream doc tool used by a lot of people, many of them are not frontend devs (technical writers, academics).
Asking our users to use custom webpack config, inline file loader syntax or anything makes the upgrade to our next version more complicated.


I'll try to figure out a solution so that moving from non-required to required does not need any change for our users, resolve.roots + default fonts file-loaders might do the trick in this case.

Still think that being able to opt-out of require() with some kind of css inline syntax could be useful, and would be perfectly fine with /* webpackIgnore: true */

Thanks

@alexander-akait
Copy link
Member

alexander-akait commented Feb 8, 2021

Supporting /* webpackIgnore: true */ is not a hard task.

We are an opiniated mainstream doc tool used by a lot of people, many of them are not frontend devs (technical writers, academics).

I know well docusaurus

But can you clarify what do you mean To keep the behavior as it was before.? Why developers write /fonts/font.ttf? Why do not provide good (resolvable) URL to fonts? Don't forget webpack has resolve.roots, so resolving is not the problem, you can have multiple roots paths

@slorber
Copy link
Author

slorber commented Feb 8, 2021

👍

To give full context, I'm working on testing our upcoming canary release on a site that requires LTR support:

After upgrading Docusaurus from @latest to local symlinks, I saw the site failed to build due to fonts:

https://github.com/massoudmaboudi/datagit_v2.docusaurus/blob/master/src/css/custom.css#L196

@font-face {
  font-family: "Iran Sans";
  font-style: normal;
  font-weight: 100;
  src: url("/fonts/IRANSans5.5/IRANSansWeb_UltraLight.eot"),
    /* IE9 Compat Modes */
      url("/fonts/IRANSans5.5/IRANSansWeb_UltraLight.eot?#iefix")
      format("embedded-opentype"),
    /* IE6-IE8 */ url("/fonts/IRANSans5.5/IRANSansWeb_UltraLight.woff2")
      format("woff2"),
    /* Super Modern Browsers */
      url("/fonts/IRANSans5.5/IRANSansWeb_UltraLight.woff") format("woff"),
    /* Modern Browsers */ url("/fonts/IRANSans5.5/IRANSansWeb_UltraLight.ttf")
      format("truetype");
} 

Errors were related to inability to resolve at those paths + missing file-loaders for fonts (after paths are fixed).

Yet the site worked fine before, as the CSS show unprocessed URLs that are correctly served: https://datagit.ir/styles.10a1b0ec.css

image

So something has changed, not sure exactly when/how :)

Currently trying to see if the resolve.roots + fonts file-loader would let the user upgrade without modifying his CSS

@alexander-akait
Copy link
Member

So something has changed, not sure exactly when/how :)

in v4 we added ability to resolve server relative url and absolute paths, so /images/image.png is now consider as resolvable

@slorber
Copy link
Author

slorber commented Feb 8, 2021

thanks for the info ;)

It seems resolve.roots is working for my usecase, so I'll let you decide if you want to implement the ignore comment

@alexander-akait
Copy link
Member

Yep, let's do it, but if no problemы right now I will reduce priority of this

@alexander-akait alexander-akait changed the title Ability for urls to not use require() on a per-url basis Support /** webpackIgnore: true */ Feb 9, 2021
@cap-Bernardito cap-Bernardito mentioned this issue Feb 18, 2021
6 tasks
@eytienne
Copy link

eytienne commented Jun 8, 2021

in v4 we added ability to resolve server relative url and absolute paths, so /images/image.png is now consider as resolvable

Is this working? Because I have Error: Can't resolve '/business-specific-image/' in '/.../scss' while having this style:

#business-specific {
  /* webpackIgnore: true */
  background-image: url("/business-specific-image/");

Is my url breaking rules to be resolved? And why the magic comment seems ignored?

@alexander-akait
Copy link
Member

You need to update css-loader, please check it

@eytienne
Copy link

eytienne commented Jun 8, 2021

Checked, I was in 5.2.4 so up to date with this change and it neither works in 5.2.6.

@alexander-akait
Copy link
Member

@eytienne please create reproducible test repo, we have strong tests on this

@eytienne
Copy link

eytienne commented Jun 8, 2021

Thank you, I fixed it trying to reproduce. It was because of postcss upstream removing comments with cssnano, the trick with ! works perfectly as mentionned here : https://cssnano.github.io/cssnano/docs/optimisations/discardcomments/

@antoniandre
Copy link

antoniandre commented Oct 3, 2023

Thank you @eytienne, saved me a lot of time!
For next readers who have had the comments stripped and it wouldn't pass the build, the final solution for me was:
/*! /* webpackIgnore: true */

@kadamwhite
Copy link

kadamwhite commented Aug 20, 2024

If anybody else runs into this, particularly if anybody else finds they can't use the /*! /* webpackIgnore: true */ directive comment either to exclude a specific url() from being processed as an import when working with the @wordpress/scripts wp-scripts CLI wrapper for Webpack, another solution: add a url option to the css-loader configuration with a filter function that excludes a specific list of assets.

If you're using a prefab Webpack config you'll need to find the relevant rule in the module.rules list, otherwise you can add the options.url value direction:

cssLoaderRuleReference.options.url = {
	/**
	 * Do not process url() statements for certain files in assets/.
	 *
	 * @param {string} url Path to asset referenced via url().
	 * @returns {boolean} Whether to process with loader.
	 */
	filter( url ) {
		return ! /assets\/(your|pattern|here)/.test( url );
	},
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants