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

Load dependencies based on the format of the parent #883

Open
desandro opened this Issue Mar 12, 2015 · 51 comments

Comments

Projects
None yet
@desandro

desandro commented Mar 12, 2015

Expanding on #333. I've had several issues opened on my packages from users having trouble using Webpack with my packages, even though I support Browserify/RequireJS/UMD with all dependencies. See desandro/masonry#679 and metafizzy/packery#239.

Package authors are set on a collision course with UMD and Webpack. Authors are encouraged to support both AMD and CommonJS exports, adhering to UMD. Ideally, when you adhere to a community specification, things should work out of the box, without requiring users to add extra config or authors to re-tool their source for your loader.

My recommendation is that Webpack should load dependencies based on the format of the parent. For example:

alpha
  - beta
  - gamma
    - delta

When you load alpha with CommonJS, all its dependencies are loaded with CommonJS. Currently, Webpack will load alpha with CommonJS then load beta with whatever loader it first detects. I feel this behavior is problematic. As an author, I expect that CommonJS or AMD loading is silo-ed. My dependencies can be loaded with either format, but not in a combination.

The current solution is use imports-loader to resolve this. I feel this is a bandage to quell the symptom.

@reggi

This comment has been minimized.

reggi commented Mar 12, 2015

When you load alpha with CommonJS, all its dependencies are loaded with CommonJS.

Brilliant 👏 👍

@sokra

This comment has been minimized.

Member

sokra commented Mar 13, 2015

Hmm I don't think it's so simple. The style of the parent is not relevant, the package manager (npm or bower) is. If a user likes to write AMD, but installs modules via npm this will break.

You assume that modules installed via npm always use CommonJs and modules installed via bower always use AMD. But with webpack this is not true anymore. Both styles can be used for both package manager. I see that this leads to an issue.

The cleanest solution would be to publish different modules to npm and bower. Each one only with the correct dependencies for the package manager. But this lead to an overhead. (I would do it this way: write your module in CJS, publish it to npm and use webpack to compile an UMD version for bower ;), this way you could split your modules into multiple files )

Here are some workarounds which would eliminate the configuration by the user:

  • Add a browser field to package.json and alias the bower dependencies to the npm ones. Exclude package.json from bower publishing.
  • Switch the order of AMD and CJS in your UMD part and hope that webpack users always install your modules via npm (I think the most do so).
@desandro

This comment has been minimized.

desandro commented Mar 13, 2015

Thanks for looking this over.

What I do assume is that if you load alpha with CommonJS, and alpha depends on beta, then beta will be loaded with CommonJS. I feel this is intuitive behavior. Webpack's current behavior feels problematic.

@sokra

This comment has been minimized.

Member

sokra commented Mar 13, 2015

What I do assume is that if you load alpha with CommonJS, and alpha depends on beta, then beta will be loaded with CommonJS. I feel this is intuitive behavior.

Yeah, but this is not correct. If you write your code in AMD you may still want to require a CommonJs module from npm. Of if you write your code in CommonJs you may still want to require a AMD module from bower...

@jhudson8

This comment has been minimized.

jhudson8 commented Mar 26, 2015

@sokra is there a way to change the loader order so that CommonJS is always checked first?

I'm having an issue which seems related to this one in the fact that I'm using exoskeleton without jquery. While jquery is not a requirement, it is a dependency from an amd standpoint (but not necessary from a CommonJS standpoint)

AMD jquery dependency
https://github.com/paulmillr/exoskeleton/blob/master/exoskeleton.js#L12

Why it is not necessarily required
https://github.com/paulmillr/exoskeleton/blob/master/exoskeleton.js#L22

@farneman

This comment has been minimized.

farneman commented Jul 28, 2015

+1 for a solution to this.

@NeXTs

This comment has been minimized.

NeXTs commented Oct 10, 2015

+1

@mguijarr

This comment has been minimized.

mguijarr commented Oct 23, 2015

+1

2 similar comments
@thesublimeobject

This comment has been minimized.

thesublimeobject commented Nov 11, 2015

+1

@cmalven

This comment has been minimized.

cmalven commented Nov 14, 2015

+1

@bebraw bebraw added the enhancement label Nov 15, 2015

@sidharthachatterjee

This comment has been minimized.

sidharthachatterjee commented Nov 17, 2015

+1

@slashwhatever

This comment has been minimized.

slashwhatever commented Nov 19, 2015

+1

@farneman

This comment has been minimized.

farneman commented Nov 29, 2015

Would it be possible to add a Webpack config option to let a user specify that they prefer a particular dependency type over others if a module exposes multiple (ie. UMD)? This could also include an option to use a dependency's parent type as the preferred type.

@desandro desandro referenced this issue Dec 2, 2015

Closed

Webpack problem #979

@kepek

This comment has been minimized.

kepek commented Dec 4, 2015

+1

1 similar comment
@jordanegstad

This comment has been minimized.

jordanegstad commented Dec 15, 2015

+1

@lbineau

This comment has been minimized.

lbineau commented Jan 5, 2016

+1

@homerjam

This comment has been minimized.

homerjam commented Jan 7, 2016

What about if a module is unresolved via AMD trying again with forced CommonJS? Would there be possibility for a plugin that does this?

@farneman

This comment has been minimized.

farneman commented Jan 10, 2016

@homerjam: That would not solve the issue here. This issue occurs when Webpack loads a UMD module that successfully resolves AMD. The problem is loading such a module from a CommonJS context still gets the AMD version since that's the first available from the UMD wrapper.

@devil1991

This comment has been minimized.

devil1991 commented Jan 11, 2016

+1

@danez

This comment has been minimized.

Contributor

danez commented Jan 20, 2016

I don't think this is necessarily a problem of webpack, but rather other packages (and as far as I see most of them are from @desandro) who assume that AMD === bower/requirejs/... and CJS === npm/node/..., which is not correct, as with all the browser-bundlers, both ways can be used in different ways.

The cleanest solution would be to publish different modules to npm and bower. Each one only with the correct dependencies for the package manager. But this lead to an overhead. (I would do it this way: write your module in CJS, publish it to npm and use webpack to compile an UMD version for bower ;), this way you could split your modules into multiple files )

I think this is the best solution.

@bboydflo

This comment has been minimized.

bboydflo commented Apr 25, 2016

+1

@NeXTs

This comment has been minimized.

NeXTs commented Apr 25, 2016

Github announced +1 feature, please use it

@k41n

This comment has been minimized.

k41n commented May 31, 2016

👍

2 similar comments
@t1m0n

This comment has been minimized.

t1m0n commented Jun 15, 2016

+1

@devboxr

This comment has been minimized.

devboxr commented Jun 28, 2016

+1

@jolane

This comment has been minimized.

jolane commented Jul 15, 2016

+1

1 similar comment
@maxdelia

This comment has been minimized.

maxdelia commented Oct 2, 2016

+1

@fgilio

This comment has been minimized.

fgilio commented Nov 30, 2016

+1 please

@misterfitzy

This comment has been minimized.

misterfitzy commented Nov 30, 2016

+1

@apokyro

This comment has been minimized.

apokyro commented Dec 16, 2016

+1 it's essential!

@pacanionio

This comment has been minimized.

pacanionio commented Jan 13, 2017

+1

17 similar comments
@mirkojotic

This comment has been minimized.

mirkojotic commented Jan 14, 2017

+1

@nsh-core

This comment has been minimized.

nsh-core commented Feb 26, 2017

+1

@brianedelman

This comment has been minimized.

brianedelman commented Mar 6, 2017

+1

@medfreeman

This comment has been minimized.

medfreeman commented Apr 6, 2017

+1

@SomosAMambo

This comment has been minimized.

SomosAMambo commented Apr 13, 2017

+1

@craigster1991

This comment has been minimized.

craigster1991 commented May 3, 2017

+1

@dominicjameskelly

This comment has been minimized.

dominicjameskelly commented May 17, 2017

+1

@marcus-herrmann

This comment has been minimized.

marcus-herrmann commented May 27, 2017

+1

@DasNaughtie

This comment has been minimized.

DasNaughtie commented Sep 8, 2017

+1

@patrickbjohnson

This comment has been minimized.

patrickbjohnson commented Sep 23, 2017

+1

@HauntedSmores

This comment has been minimized.

HauntedSmores commented Oct 2, 2017

+1

@medero

This comment has been minimized.

medero commented Oct 4, 2017

+1

@kirylpl

This comment has been minimized.

kirylpl commented Oct 4, 2017

+1

@jimblue

This comment has been minimized.

jimblue commented Oct 7, 2017

+1

@Robula

This comment has been minimized.

Robula commented Oct 18, 2017

+1

@cbmaiolo

This comment has been minimized.

cbmaiolo commented Oct 19, 2017

+1

@tderleth

This comment has been minimized.

tderleth commented Dec 12, 2017

+1

@prewk

This comment has been minimized.

prewk commented Jan 30, 2018

Somebody has to say it: Stop posting "+1" on GitHub!

The emoji buttons are there for a reason..

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