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

accept custom resolve function #1441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cactusbone
Copy link

custom resolve function which default to internal resolve when not found.

allows to easily plug in some files.

not far from #1006 though

along with module_deps fileCache, can be used to provide generated content for #1436

@jmm
Copy link
Collaborator

jmm commented Nov 20, 2015

Interesting approach @Cactusbone. I've been thinking about a good way to solve that (allow setting it, but also wrapping the original). Part of the issue for me is that I wish to be able to do it from a plugin.

@Cactusbone
Copy link
Author

from what I understood, plugins can interact only after modules_deps right now, so it's not gonna be easy :)

@jmm
Copy link
Collaborator

jmm commented Nov 20, 2015

@Cactusbone That's true. It could be as simple as just documenting .resolver as public API on module-deps in combination with allowing it to be passed in as an opt. I have done some work on prototyping an extension to the plugin API that would allow plugins to run before the pipeline is constructed. I think all you'd lose is the ability to synchronously access b.pipeline before b.bundle().

@Cactusbone
Copy link
Author

.resolver did not do the trick for me, since it still required existing files and i wanted to give arbitrary code.

@jmm
Copy link
Collaborator

jmm commented Nov 23, 2015

@Cactusbone I'm not sure what you mean. Wrapping .resolver doesn't inherently require existing files any more than this PR does, does it?

@Cactusbone
Copy link
Author

@jmm I don't remember exactly from my tests, but browser-resolve was checking if my files existed when i used .resolver, whereas my PR changes directly in module-deps, which check first in its fileCache.

check #1436 for my usage :)

I can check again if you want the exact stack trace

@jmm
Copy link
Collaborator

jmm commented Nov 23, 2015

@Cactusbone I understand what you're going for, I'm just not sure what difference there would be between what you've PR'ed here and this:

var b = browserify();
var mdeps = b.pipeline.get("deps").get(0);
var baseResolve = mdeps.resolver;
mdeps.resolver = function (id, parent, cb) {
  customResolve(id, parent, function (err, file, pkg) {
    if (!err && !file) return baseResolve(id, parent, cb);
    else return cb(err, file, pkg);
  });
};

In your PR opts.resolve just becomes .resolver, doesn't it?

(Please note that I'm not necessarily saying your proposal shouldn't be considered -- there's a couple of different issues involved here.)

@Cactusbone
Copy link
Author

@jmm looks like my memory was playing against me. indeed using a customResolver here do work as well as my PR :)

@jmm
Copy link
Collaborator

jmm commented Nov 23, 2015

@Cactusbone Ok, thanks for the feedback. I personally do think there should be public API for using a custom resolver eventually -- I would just want to see it have a sensible API (including being able to set it via plugin, for example).

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

Successfully merging this pull request may close these issues.

None yet

2 participants