bholloway / resolve-url-loader

Webpack loader that resolves relative paths in url() statements based on the original source file
563 stars 71 forks source link

Reinstate file search magic #129

Closed runfaj closed 3 years ago

runfaj commented 4 years ago

We recently have started migrating from webpack 3 to 4, and included in that is upgrading this package from 2 to 3. In our setup, we have many scss files that reference image urls like so: url('images/<image>').

V2

This images reference is a resolved path that is aliased as src/client/images. In version 2 of the resolve url loader, in the eachSplitOrGroup function, the absolute variable (absolute = uri && findFile(options).absolute(directory, uri, resolvedRoot)) used a findFile method where it appears to do a queued recursive search going up the file structure until it finds the file.

In that findFile case, this worked well because it would eventually walk up the tree until it reaches src/client and it finds the appended path src/client/images and ultimately finds the absolute path the the file src/client/images/<image>.

After that path is found, the following code runs and has the appropriate replacement with the relative path from the original file to the found absolute path.

var relative     = absolute && path.relative(filePath, absolute),
    rootRelative = relative && loaderUtils.urlToRequest(relative, '~');
return (rootRelative) ? rootRelative.replace(BACKSLASH_REGEX, '/').concat(query) : initialised;

V3

However, in V3, this isn't the case. The absolute variable mentioned above is now replaced by running the join method: absolute = testIsRelative(uri) && join(uri, candidate) || testIsAbsolute(uri) && join(uri). This join method, instead of returning the appropriate path to src/client/images/<image>, just resolves to <sourceFilePath>/images/<image>. This is a result of the default join using a simpleJoin which just concats the paths together.

Fix

Now, I'm sure I can rewrite the join method to meet my needs, but it seems like that behavior that existed in v2 should be re-added as a default instead of the "simpleJoin" method.

If not, at least the documentation should specify what this join method does, what the outputs will default to, and an example of how to write a custom one. It should also specify this as a breaking change, as it indeed does break just about every one of our scss files that use the existing V2 logic.

runfaj commented 4 years ago

If it helps, here's the original find-file that was doing the recursive search. https://github.com/bholloway/resolve-url-loader/blob/4d8df6d250ef870b70fdf125d041eaa95f597db1/lib/find-file.js

Though not as elegant, here's my join method currently to somewhat replicate the needed functionality. Could definitely be optimized like the original (in the link above) to filter out things above the root path and such.

function doResolveJoin(fileName, options) {
  return function(uri, candidate) {
    ////skip data: type urls
    if (_.includes(uri, 'data:')) {
      return uri;
    }

    ////if already absolute
    if (path.isAbsolute(uri)) {
      return uri;
    }

    ////if already relative, convert to absolute
    if (_.startsWith(fileName, '.')) {
      return path.resolve(uri);
    }

    ////else, search for matching module path by walking up tree
    let pathsToTry = [];

    //search fileName path first - better chance of matching
    let pathParts = fileName.split('/');
    pathParts.pop(); //remove actual file scss file reference, so it is path only
    while (pathParts.length > 0) {
      pathsToTry.push(pathParts.join('/'));
      pathParts.pop();
    }

    //then try candidate path search
    pathParts = candidate.split('/');
    while (pathParts.length > 0) {
      pathsToTry.push(pathParts.join('/'));
      pathParts.pop();
    }

    //gathered potential path list, now just check until we find a match
    while (pathsToTry.length > 0) {
      let testPath = path.join(pathsToTry[0], uri);
      if (fs.existsSync(testPath)) {
        return testPath;
      }
      pathsToTry.shift();
    }

    ////couldn't find any matches - must be a user typed error
    return uri;
  };
}
bholloway commented 4 years ago

Is the full file search required in your use case or can you just rebase all your urls to image directory?

runfaj commented 4 years ago

Is the full file search required in your use case or can you just rebase all your urls to image directory?

We recently moved all of the image files to 2 directories. However, we used to have "images" directories at many levels, so the tree walking search was necessary to get the closest match.

bholloway commented 4 years ago

@runfaj to clarify are you currently making do with the doResolveJoin that you posted?

The old file search is still in this mono-repo but it has not been adapted to the new join api. I am open to publishing it if you can adapt and test it.

runfaj commented 4 years ago

@bholloway Yes, that code above is working well for our use case currently. That is just at the bottom of our webpack file and referenced via the join option.

I'm open to testing anything you publish for this - however, it might be worth optimizing, like the V2 version had, to speed up the searching potentially.

bholloway commented 4 years ago

@runfaj I'll keep this open as an enhancement. I will be interested to see how many other people are in this situation.

Also beware that in an upcoming update I will be sending the join function an iterator instead of a string. You will see in the reference join function it can handle a number of possible base paths. This will allow url declarations containing sass variables to work correctly.

🤔 I was thinking about making that update a major version bump. It would be a breaking change for your join function at least.

bholloway commented 4 years ago

@runfaj out of interest what is your engine setting - is it engine: "postcss" or engine: "rework" or unspecified?

runfaj commented 4 years ago

@bholloway Thanks for the update! I'll keep on the lookout for the upcoming changes - it will be easy enough on our side to update the webpack config as needed.

We had the issue with both engines, but currently have it unspecified.

MightyPork commented 4 years ago

I just updated to v3 and everything broke :ok_hand: We were using urls defined in variables as a way to implement different themes, e.g.

scss/
+ themes/
  + theme1_files/
    + logo.svg
  + theme2_files/
    + logo.svg
  + _theme1.scss
  + _theme2.scss
+ app.scss

app.scss is patched by a pre-build script to import the correct theme.

the theme contains lines such as:

// relative urls never worked here - theme1_files/logo.svg
$logo: url('themes/theme1_files/logo.svg');

the variables are then used in various subfolders where components etc. are defined.

This worked before, but now the plugin can't find the file and crashes the build. I think I'll have to hack together some custom join function, but it looks very intimidating.

I tried setting "root", but it then tries to process real absolute urls that shouldn't be touched.

bholloway commented 4 years ago

@mightypork as another data point try the v4 alpha using @next.

MightyPork commented 4 years ago

@bholloway do you mean "4.0.0-alpha.1"? Installed that and I see no change. Is there some new config needed to make variables work?

bholloway commented 4 years ago

@MightyPork the only precondition is that you use engine:postcss or don't specify engine as postcss is default.

I take your point with the custom join. The code is not my best. I'm hoping to re-release the search routine for v4 but its an optional goal.

But fundamentally the custom join would be better since the search is just not performant or clearly deterministic. If you can produce a minimum breaking repo I hope to understand your use case better and maybe help with a custom join.

MightyPork commented 4 years ago

@bholloway yeah ok, I had the engine set to 'rework' from some old config.

It almost works now, just one image stubbornly refuses to resolve. It's not in a mixin or anything strange, either. In my case, if we could pass the loader a key-value map with relative to absolute URL mappings (absolute from the sass root), that'd be a quick and easy fix here. Alternatively, some "fallback" resolve function that simply receives the file path and relative path and can try its best to find it.

bholloway commented 4 years ago

@MightyPork for urls that are exceptions you would want to use some sort of postcss-loader step I would think. For further discussion please open a separate issue for "exceptions" feature request.

Given your partial success please look at #126 (trying to avoid linking the issue) and see if it better represents your situation. If so please follow that issue for v4 updates.

MightyPork commented 4 years ago

@bholloway I'm not really sure what's the cause, since I'm not using a mixin and my url()s are inside the variables.. but regardless, I have fixed it using a customized join.

here's the hack if anyone else hits the same issue:

let fs = require('fs');
let RUL = require('resolve-url-loader');
let compose = require('compose-function');
let simpleJoin = compose(path.normalize, path.join); // not exported :/

let theme_base = path.join(__dirname, 'resources/assets/sass/theme')

let joinHack = RUL.createJoinForPredicate(
  function predicate(filename, uri, base, i, next) {
    //console.log('>>>>>>>>>>>>>>>>>>>> ', filename, uri, base, i)

    let absolute = simpleJoin(base, uri);
    if (fs.existsSync(absolute)) return absolute;

    // try using the theme folder as base
    let theme_path = simpleJoin(theme_base, uri);
    if (fs.existsSync(theme_path)) return theme_path;

    return next((i === 0) ? absolute : null);
  },
  'customJoin'
)
bholloway commented 4 years ago

Nice one @MightyPork. 👍

I'm open to tweaking the join funciton for v4. I think there is only a small cohort of users using the custom join, and they are (hopefully) all watching this issue.

bholloway commented 4 years ago

@runfaj I'm hoping that if we restore the file search code that it should be a short term solution for 99% of people.

Thinking of some use cases...

My point is that these are transient states in a migration and we should be helping/pushing the migration. Is that also true of your case?

runfaj commented 4 years ago

@bholloway Yes -- in our case, we heavily rely on aliased pathing as some of our path routes are 10 levels deep and trying to use absolute or relative pathing from the bottom up is nearly impossible to read, especially when it might be something aliased near the specified alias root (or roots). We've never had any packages with issues on pathing in our application, so I imagine that use case is pretty small. Ideally, the default case would be to maybe not enable a file search by default, as it can be much slower, but it definitely still needs to be an option.

bholloway commented 4 years ago

@MightyPork @runfaj I have a PR for refactoring the join function #149.

I think it addresses @MightyPork case most easily.

Potentially we can add in some rudimentary file search util in the same file. Although I'm thinking there has to be some iterative file-system walker package out there already!? 🤔

runfaj commented 4 years ago

Potentially we can add in some rudimentary file search util in the same file. Although I'm thinking there has to be some iterative file-system walker package out there already!? 🤔

@bholloway, there's several find packages that could maybe be utilized. Here's one example of how I was handling in my resolving: https://www.npmjs.com/package/find-up. You'd definitely want to find from the bottom up since an alias to a file could be found at multiple levels, so you'd want to get the closest match first. But maybe that also brings up the question if an extra flag for how to match (bottom up vs top down) should be implemented.

bholloway commented 3 years ago

An update on this issue.

Following #149 merging I stated

I think it addresses @MightyPork case most easily.

Potentially we can add in some rudimentary file search util in the same file. Although I'm thinking there has to be some iterative file-system walker package out there already!? 🤔

I realise v4 is still not released so I want to limit scope.

The current plan is to add some docs on how to customise the join function. Show a rudimentary file search example with find-up or something similar but not actually implement in this package. If anyone wants to collaborate on docs please let me know here.

bholloway commented 3 years ago

Some docs and refactoring in #188.

I have adjusted the original file-system search packages/resolve-url-loader-filesearch to the new generator format but it is untested.

Please feel free to join the PR and comment on the docs and/or the refactor.

bholloway commented 3 years ago

The PR #188 has now merged and there is a new release resolve-url-loader@4.0.0-alpha.3.

Let's continue any discussion of the custom join here.

MightyPork commented 3 years ago

I'm trying to use the new generator function, but can't get it to work. Must be some silly mistake on my side. Does this look correct?

const {
  createJoinFunction,
  defaultJoinOperation
} = require('resolve-url-loader');
const fs = require('fs');

let theme_base = path.join(__dirname, 'resources/assets/sass/theme')

const myGenerator = function* (filename, uri, isAbsolute, bases, options) {
  if (isAbsolute) {
    yield options.root;
  } else {
    let joined = path.normalize(path.join(bases.value, uri));
    if (fs.existsSync(joined)) {
      console.log('Resolved1 -> ', joined)
      yield joined;
    } else {
      joined = path.normalize(path.join(theme_base, uri));
      if (fs.existsSync(joined)) {
        console.log('Resolved2 -> ', joined)
        yield joined;
      } else {
        console.warn('NOT FOUND!', uri)
      }
    }
  }
}

const myJoinFn = createJoinFunction({
  name     : 'myJoinFn',
  scheme   : 'alstroemeria',
  generator: myGenerator,
  operation: defaultJoinOperation
});

// Update the webpack settings in laravel mix
mix.override((config) => {
  // patch resolve-url-loader config
  config.module.rules.forEach((r) => {
    if (typeof r.test === 'string' && r.test.endsWith("app.scss")) {
      r.use.forEach((ld) => {
        if (ld.loader === 'resolve-url-loader') {
          ld.options.debug = true;
          ld.options.engine = 'postcss';
          ld.options.join = myJoinFn;
        }
      });
    }
  })
})

It looks like it's working, my NOT FOUND! is never printed. Yet, css-loader then fails.

logs ``` resolve-url-loader: ./resources/assets/sass/app.scss: fb/logo_std_full.svg ./resources/assets/sass/theme/fb/logo_std_full.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_full.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_small.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb/logo_std_small.svg ./resources/assets/sass/theme/fb/logo_std_small.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_full.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_small.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb/select_white.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb/select_white.svg ./resources/assets/sass/theme/fb/select_white.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb_yellow/hextile-large-a.svg ./resources/assets/sass/theme/fb_yellow/hextile-large-a.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb_yellow/hextile-large-p.svg ./resources/assets/sass/theme/fb_yellow/hextile-large-p.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade` 70% building 607/608 modules 1 active ...js??ref--5-4!/srv/flowbox/node_modules/sass-loader/dist/cjs.js??ref--5-5!/srv/flowbox/resources/assets/sass/app.scssResolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_full.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb/logo_std_full.svg ./resources/assets/sass/theme/fb/logo_std_full.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_full.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_small.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb/logo_std_small.svg ./resources/assets/sass/theme/fb/logo_std_small.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_full.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb/logo_std_small.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb/select_white.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb/select_white.svg ./resources/assets/sass/theme/fb/select_white.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb_yellow/hextile-large-a.svg ./resources/assets/sass/theme/fb_yellow/hextile-large-a.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg resolve-url-loader: ./resources/assets/sass/app.scss: fb_yellow/hextile-large-p.svg ./resources/assets/sass/theme/fb_yellow/hextile-large-p.svg NOT FOUND Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved1 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-p.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg Resolved2 -> /srv/flowbox/resources/assets/sass/theme/fb_yellow/hextile-large-a.svg ... ERROR in ./resources/assets/sass/app.scss Module build failed (from ./node_modules/css-loader/index.js): ModuleNotFoundError: Module not found: Error: Can't resolve './theme/fb/logo_std_full.svg/fb/logo_std_full.svg' in '/srv/flowbox/resources/assets/sass' at /srv/flowbox/node_modules/webpack/lib/Compilation.js:925:10 at /srv/flowbox/node_modules/webpack/lib/NormalModuleFactory.js:401:22 at /srv/flowbox/node_modules/webpack/lib/NormalModuleFactory.js:130:21 at /srv/flowbox/node_modules/webpack/lib/NormalModuleFactory.js:224:22 at /srv/flowbox/node_modules/neo-async/async.js:2830:7 at /srv/flowbox/node_modules/neo-async/async.js:6877:13 at /srv/flowbox/node_modules/webpack/lib/NormalModuleFactory.js:214:25 at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:213:14 at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :15:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7 at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :15:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :27:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43 at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :16:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :27:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43 at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :16:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/Resolver.js:285:5 at eval (eval at create (/srv/flowbox/node_modules/tapable/lib/HookCodeFactory.js:33:10), :15:1) at /srv/flowbox/node_modules/enhanced-resolve/lib/DirectoryExistsPlugin.js:27:15 at /srv/flowbox/node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:85:15 at processTicksAndRejections (node:internal/process/task_queues:76:11) @ ./resources/assets/sass/app.scss ERROR in ./resources/assets/sass/app.scss (./node_modules/css-loader??ref--5-2!./node_modules/postcss-loader/src??postcss0!./node_modules/resolve-url-loader??ref--5-4!./node_modules/sass-loader/dist/cjs.js??ref--5-5!./resources/assets/sass/app.scss) Module not found: Error: Can't resolve './theme/fb/logo_std_full.svg/fb/logo_std_full.svg' in '/srv/flowbox/resources/assets/sass' @ ./resources/assets/sass/app.scss (./node_modules/css-loader??ref--5-2!./node_modules/postcss-loader/src??postcss0!./node_modules/resolve-url-loader??ref--5-4!./node_modules/sass-loader/dist/cjs.js??ref--5-5!./resources/assets/sass/app.scss) 7:53360-53420 7:62679-62739 7:68146-68206 @ ./resources/assets/sass/app.scss ERROR in ./resources/assets/sass/app.scss (./node_modules/css-loader??ref--5-2!./node_modules/postcss-loader/src??postcss0!./node_modules/resolve-url-loader??ref--5-4!./node_modules/sass-loader/dist/cjs.js??ref--5-5!./resources/assets/sass/app.scss) Module not found: Error: Can't resolve './theme/fb/logo_std_small.svg/fb/logo_std_small.svg' in '/srv/flowbox/resources/assets/sass' @ ./resources/assets/sass/app.scss (./node_modules/css-loader??ref--5-2!./node_modules/postcss-loader/src??postcss0!./node_modules/resolve-url-loader??ref--5-4!./node_modules/sass-loader/dist/cjs.js??ref--5-5!./resources/assets/sass/app.scss) 7:62979-63041 7:68755-68817 @ ./resources/assets/sass/app.scss ... ```

How did it even arrive at "./theme/fb/logo_std_full.svg/fb/logo_std_full.svg"? So I'm back to my old join hack for now

bholloway commented 3 years ago

@MightyPork thanks for trying this. I think its close.

So in this new concept the generator is expected to just order the base paths and the operator takes the next base and tests to see if it is suitable. The only problem I see in your generator is that you need to yield the base path, not the joined path.

Notice the yield base; in this example its easy to miss and I should document it better.

So you can stick to that generator but in your case I think you could make it even simpler. All you need to do is to return your theme path as another base path for the loader to try. Meaning this.

const myGenerator = (_filename, _uri, isAbsolute, bases, options) =>
  isAbsolute? [ options.root ] : [ bases.value, theme_base ];

So the loader will try bases.value first and if that doesn't work it will try theme_base. In both cases the operation will append the uri and test if the file exists, so you don't need to do that part.

Although you might find it useful to try all the other locations in bases before falling back to the theme. Meaning this.

const myGenerator = (filename, uri, bases, isAbsolute, options) =>
  isAbsolute ? [options.root] : [bases.subString, bases.value, bases.property, bases.selector, theme_base];
bholloway commented 3 years ago

🤔 Actually this "theme" use-case is probably a good example to document.

MightyPork commented 3 years ago

Nice, that worked. Here's the winner:

let theme_base = path.normalize(path.join(__dirname, 'resources/assets/sass/theme'));

const myJoinFn = createJoinFunction({
  name     : 'myJoinFn',
  scheme   : 'alstroemeria',
  generator: (_filename, _uri, isAbsolute, bases, options) =>
    isAbsolute? [ options.root ] : [ ...Object.values(bases), theme_base ],
  operation: defaultJoinOperation
});

I spent an embarrassing amount of time with [...bases, theme_base], which obviously didn't work, but with a very non-obvious error.

bholloway commented 3 years ago

It's awesome that the new concept of generator and operator cuts out so much code! 🎉

I suspected that would be the cases since most people I think just want to add more search paths.

...Object.values(bases)

Hmm interesting, I guess it should enumerate in deterministic order. You could also call the default implementation and just add your theme on the end. That would guarantee the default precedence of source-map locations.

const myJoinFn = createJoinFunction({
  ...,
  generator: (...args) => [ ...defaultJoinGenerator(...args), theme_base ],
  operation: defaultJoinOperation
});

I think the only difference would be that your theme would also apply to absolute URIs. Which is not ideal I guess.

Honestly I am not sure if we should be catering for absolute URIs at all. Or we somehow configure them different generator or something. I think in most cases people will not care to change the default behaviour of options.root.

bholloway commented 3 years ago

Out of interest @MightyPork do you prefer a hash of arguments?

Meaning something like this.

const myGenerator = (( filename, uri, isAbsolute, bases }, options) => ...

I keep flip-flopping the API because I can't really decide what is better at this state. I have the scheme specifically so it can be changed later but still interested it getting something right first time.

My thinking is that it would allow something like this.

const myJoinFn = createJoinFunction({
  ...,
  generator: (args, options) => {
    const { isAbsolute } = args;
    const recursed = defaultJoinGenerator(args, options);
    return isAbsolute ? recursed : [ ...recursed, theme_base ];
  },
  operation: defaultJoinOperation
});

Although now I look at it its pretty obtuse.

MightyPork commented 3 years ago

I don't think it really matters that much, this is the kind of code you write/copy from examples once per project and then forget about it.

A nice thing with the hash is that the arguments are named (so you can't have them swapped by mistake like in the generator on your example page). I would put options inside the hash as well if you go this way, so it's consistent. But either way is okay imo. By far the most confusing bit of your new API is the gibberish you need to use as a "schema" ;)

bholloway commented 3 years ago

Sorry @MightyPork more churn on the "join" function API. See the revised docs. I think it should be mostly in line with your feedback though. 👍

Published as resolve-url-loader@4.0.0-alpha.4.

Should be easy conversion for you. 🤞 Your use case is one of the documented examples now. 🎉

bholloway commented 3 years ago

Released resolve-url-loader@4.0.0-beta.1.

Since v4 is now master branch and v3 has moved to maintenance so I'm going to close this issue.

If you cannot adopt the beta in your project, or v4 full release is not released in the next few weeks, then feel free to reopen this issue.

runfaj commented 2 years ago

@bholloway Sorry about the awfully late reply, but thanks to your examples and working through this finally, I was able to match my original needed condition with the "walk up" method and tweak your example (which appeared to have a couple typos). Here's what ended up working for the generator:

const doResolveJoin = createJoinFunction(
  'doResolveJoin',
  createJoinImplementation(resolveGenerator)
);

// search up from the initial base path until you hit a package boundary
function* resolveGenerator(
  { uri, isAbsolute, bases: { subString, value, property, selector } }, // the example had a typo here for the subString
  { root, attempts = 1e3 },
  { fs }
) {
  if (isAbsolute) {
    yield [root, uri];
  } else {
    for (let base of [subString, value, property, selector]) {
      // don't allow searching above folder root level
      if (base.split('/').length < __dirname.split('/').length) { // added this from the example, since that was one of the mentions
        continue;
      }

      for (let isDone = false, i = 0; !isDone && i < attempts; i += 1) {
        yield [base, uri];
        // unfortunately fs.existsSync() is not present so we must shim it
        const maybePkg = path.normalize(path.join(base, 'package.json'));
        try {
          isDone = fs.statSync(maybePkg).isFile();
        } catch (error) {
          isDone = false;
        }
        base = base.split('/').slice(0, -1).join('/'); // this wasn't working in the example, so had to adjust the splitting
      }
    }
  }
}

Thanks for the work on this package!

MightyPork commented 2 years ago

Took a while, but I finally updated our webpack config and libraries and this time it worked, so I'm keeping it. -> so now really using v4 of this loader. I can't say I understand it, but your example worked without changes, thanks @bholloway :+1:

should be noted that the URL in the above post is wrong now, it should be https://github.com/bholloway/resolve-url-loader/blob/v4-maintenance/packages/resolve-url-loader/docs/advanced-features.md