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

Browserify includes required from modules defined as external #401

Closed
amitayd opened this Issue May 15, 2013 · 10 comments

Comments

Projects
None yet
5 participants
@amitayd
Copy link

amitayd commented May 15, 2013

(Transitive dependency in this context means module A requires module B, and module B requires module C. Hence, module C is a transitive dependency of A).

When module A has a transitive dependency only module C through module B which is external (not included), browserify still includes that dependency (C).

I think it should be expected from the external bundle to include those dependencies, instead of having to manually externalize each of them.

I have included an extension of the docs example (beep, boop, robot) for what I mean at https://gist.github.com/amitayd/5580837 .

Currently, browserify beep.js -x robot.js would include beep.js and circuit.js (which is only used by robot.js).

I believe the expected behaviour including only beep.js.

Implementation notes

I only had a brief look at the code base so i might be entirely wrong, but it seems that the list of files and their dependency is gathered from module-deps for all required files, and the returned list does not include the origins/requiree(s) of the dependency, so it's a problem to filter them in browserify.

One solution is to pass the externals filter to be used by module-deps when walking the dependencies.

Another is the module-deps can return something like origin modules array for each row, then browserify can work like this:
If all of row dependencies are external, skip.

@go-oleg

This comment has been minimized.

Copy link

go-oleg commented May 16, 2013

+1...I am running into this issue too.

Thanks for browserify!

@amitayd

This comment has been minimized.

Copy link

amitayd commented May 16, 2013

If a core contributor would provide me with some design principles such as "can module-deps filter by externals" I might be able to contribute a fix.

@dominictarr

This comment has been minimized.

Copy link
Collaborator

dominictarr commented May 21, 2013

I contributed a patch to the module deps system recently, so I have some familiarity with it.
before, it filtered modules out after it resolved them -- this caused problems if the module didn't actually exist.

I added opts.filter function because I needed something that filtered out things before they where resolved.
this should mean that you could use it to exclude modules.

so, exclude works by filtering out the modules that module-deps finds, but if you used the filter option, you should get the behaviour you desire.

https://github.com/substack/node-browserify/blob/master/index.js#L226

See how mdeps is piped to https://github.com/substack/node-browserify/blob/master/index.js#L206

@amitayd

This comment has been minimized.

Copy link

amitayd commented May 22, 2013

Thank you for your input, dominictarr.
I'm a bit confused about the opts for bundle() being passed as they are (more or less) to module_deps. Does that mean providing a opts.filter function to bundle() should be part of the API? (or did you add it to be used with module-deps externally, i.e. not related to browserify)?

So I understand I can create and pass a filter function that would make use of self._external to filter out external deps. Can anyone think of a reason why transitive dependencies shouldn't be excluded?

If passing opts.filter to bundle() should be supported I can do something like returning "original_filter (if exists) OR externalfilter".

@dominictarr

This comment has been minimized.

Copy link
Collaborator

dominictarr commented May 22, 2013

yes, I am using filter externally, but I don't see any reason that couldn't be in browserify.
not filtering transitive deps is pretty weird. If you make a change and it still passes the tests, then that will be considered to be the api...

@amitayd

This comment has been minimized.

Copy link

amitayd commented May 22, 2013

Ok, so i think for now, i'll implement it so passing opt.filter to bundle() will override the new "filter externals" filter. Thanks again.

@ichernev

This comment has been minimized.

Copy link
Contributor

ichernev commented Jul 3, 2013

I'm also interested in this fix. I haven't looked into module-deps, but it seems like the resulting bundled file has the dependencies of each file listed beneath it (like "./robot": "mj5nX9" or "./circuit": 1). And in the case of transitive deps included, they are basically not referenced from anywhere, so I guess those can be filtered pretty easily before writing the resulting bundle.

regality added a commit to regality/node-browserify that referenced this issue Jul 9, 2013

external deps are not parsed
updates browserify#401

I think this is the expected behavior for external dependencies.
We do no want their dependencies bundled, as they are with the external.

This patch adds external deps to _noParse, preventing their deps from
being included.

substack added a commit that referenced this issue Jul 9, 2013

external deps are not parsed
updates #401

I think this is the expected behavior for external dependencies.
We do no want their dependencies bundled, as they are with the external.

This patch adds external deps to _noParse, preventing their deps from
being included.
@substack

This comment has been minimized.

Copy link
Collaborator

substack commented Jul 9, 2013

PR #401 should fix this.

@substack substack closed this Jul 9, 2013

@amitayd

This comment has been minimized.

Copy link

amitayd commented Jul 10, 2013

I think you meant PR #449.

@amitayd

This comment has been minimized.

Copy link

amitayd commented Jul 10, 2013

Verified that the issue as demonstrated by the gist https://gist.github.com/amitayd/5580837 is fixed.

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