bootstrap-loader icon indicating copy to clipboard operation
bootstrap-loader copied to clipboard

resolve-url-loader no longer searches the whole package to resolve an url

Open x-yuri opened this issue 6 years ago • 9 comments

In 2.x resolver-url-loader tried every dir of a package (bootstrap-sass) to resolve an URL. In 3.x they only try corresponding css file's directory. I guess it has to do with the following entry in changelog,

Removed file search "magic" in favour of join option.

and the following dir in the repository.

One could probably use resolve-url-loader's new join option, if it weren't for the fact that it takes a function. As a result, out-of-the-box setup of bootstrap-loader with [email protected] doesn't work since it can't resolve paths to fonts.

The only workaround I see is,

.bootstraprc:

...
useCustomIconFontPath: false
preBootstrapCustomizations: bootstrap-pre-customizations.scss
...

bootstrap-pre-customizations.scss:

$icon-font-path: '../../fonts/bootstrap/';

Now that I think about it, what's the idea behind this line?

processedStyles.push(`$icon-font-path: "${getFontsPath(bootstrapRelPath, this)}";`);

What makes you think bootstrap-sass would be okay with taking path to fonts relative to bootstrap-loader's dir?

UPD Test repo (branch bootstrap-loader-resolve-url-loader), workaround.

x-yuri avatar Sep 15 '18 19:09 x-yuri

@x-yuri I'm open to your thoughts! @alexfedoseev and I are crazy busy right now. If you can make an new entry in the examples directory that passes with your changes and fails without your changes and the other cases still pass, then that will work!

justin808 avatar Sep 16 '18 01:09 justin808

@x-yuri Any update on this issue? Any recommendation?

justin808 avatar Oct 14 '18 22:10 justin808

@justin808 Basically, there are two ways:

  1. bootstrap-loader/src/bootstrap.styles.loader.js:
-processedStyles.push(`$icon-font-path: "${getFontsPath(bootstrapRelPath, this)}";`);
+processedStyles.push(`$icon-font-path: "../../fonts/bootstrap/";`);
  1. Make the user responsible for maintaining the path.

Granted that it's only about Bootstrap 3, so the path is unlikely to change, we can probably just hardcode it.

x-yuri avatar Oct 16 '18 12:10 x-yuri

@x-yuri I agree. What's the needed action? PR to merge?

justin808 avatar Oct 26 '18 03:10 justin808

@justin808

diff --git a/node_package/tests/utils/getFontsPath.test.js b/node_package/tests/utils/getFontsPath.test.js
deleted file mode 100644
index 0563f11..0000000
--- a/node_package/tests/utils/getFontsPath.test.js
+++ /dev/null
@@ -1,7 +0,0 @@
-import test from 'tape';
-import getFontsPath from '../../../src/utils/getFontsPath';
-
-test('getFontsPath works as expected', assert => {
-  assert.equals(getFontsPath('../'), '../assets/fonts/bootstrap/');
-  assert.end();
-});

diff --git a/src/bootstrap.styles.loader.js b/src/bootstrap.styles.loader.js
index 5fb278a..d5891fa 100644
--- a/src/bootstrap.styles.loader.js
+++ b/src/bootstrap.styles.loader.js
@@ -1,7 +1,6 @@
 /* eslint func-names: 0 */
 import loaderUtils from 'loader-utils';
 import processModules from './utils/processModules';
-import getFontsPath from './utils/getFontsPath';
 import createUserImport from './utils/createUserImport';
 import createBootstrapImport from './utils/createBootstrapImport';
 import logger from './utils/logger';
@@ -44,7 +43,7 @@ module.exports = function() {
   processedStyles.push(createBootstrapImport('variables', bootstrapVersion, bootstrapRelPath));
 
   if (bootstrapVersion === 3 && !useCustomIconFontPath) {
-    processedStyles.push(`$icon-font-path: "${getFontsPath(bootstrapRelPath, this)}";`);
+    processedStyles.push(`$icon-font-path: "../../fonts/bootstrap/";`);
   }
 
   if (bootstrapCustomizations) {

diff --git a/src/utils/getFontsPath.js b/src/utils/getFontsPath.js
deleted file mode 100644
index 7330639..0000000
--- a/src/utils/getFontsPath.js
+++ /dev/null
@@ -1,9 +0,0 @@
-import path from 'path';
-
-/**
- * Builds path to Bootstrap fonts
- *
- * @param {string} bootstrapRelPath
- * @returns {string}
- */
-export default bootstrapRelPath => path.join(bootstrapRelPath, 'assets', 'fonts', 'bootstrap/');

That probably is all.

x-yuri avatar Oct 31 '18 23:10 x-yuri

@x-yuri if you can put that in a PR, I'll merge. 👍

justin808 avatar Oct 31 '18 23:10 justin808

By the way the README says:

$icon-font-path: ../fonts // relative to your Sass file

But I bet the path is to be relative to node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_glyphicons.scss, since it's it that "imports" the font (has url(...)). resolve-url-loader resolves paths relative to a file that "imports" the resources, not relative to the one that defines variables used in url(...).

On production it might get even more trickier, since node_modules is generally symlinked from elsewhere.

UPD. Indeed $icon-font-path is to be relative to node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_glyphicons.scss (branch bootstrap-loader-custom-font). The issue weren't introduced by [email protected] (branch bootstrap-loader-custom-font-v2). And in production environment $icon-font-path is to be different from that of development environment. (branch bootstrap-loader-custom-font-symlink).

x-yuri avatar Nov 01 '18 07:11 x-yuri

@x-yuri thank you for this workaround!

FYI anyone coming to this now, for this to work for me I have to use the bootstrapCustomizations key in .bootstraprc not preBootstrapCustomizations (@x-yuri uses bootstrapCustomizations in the example repo, but mentions preBootstrapCustomizations in the original message).

jwhitmarsh avatar Jan 16 '19 11:01 jwhitmarsh

If there's a PR for a doc or code fix, I'd be happy to merge it. 😄

justin808 avatar Dec 02 '19 09:12 justin808