-
Notifications
You must be signed in to change notification settings - Fork 602
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
Bundling with Webpack doesn't work well #441
Comments
Hello, thanks for your interest and for submitting this issue. I haven't looked deep into it yet. But, as far as I understood, webpack parses the file, finds every occurrence of My first question, why doesn't it consider the cjs information rather than the AMD one from the UMD? |
Some other curiosity-questions... Does wekpack try to include the modules below? a) // A:
if ( false ) {
require( "this-module-should-never-be-loaded" );
}
// B:
// require( "commented-out-module" ); |
Thanks for the quick response!
exactly, and then concats it all together, its similar to the requirejs pre-processor step, except that it supports all sorts of module/file types and is plug-able for fancier things.
It supports amd, so it doesn't mind loading it, however, the npm package doesn't install a dependency called
both of those approaches work in webpack. It is smart enough to not try in the case of
|
cc @jzaefferer @scottgonzalez for input. |
Have you talked to the webpack team about this? If Globalize is working properly on its own for CJS and AMD environments, it sounds like webpack has a bug. |
hey @scottgonzalez thanks for helping out! Its definately not a webpack bug. The underlying issue is that Globalize defines a dependancy that doesn't exist in the npm package. The webpack "environment" is unique because its one of the few places that both module types are valid , so the UMD wrapper never fails on any of its checks. The reason it works in node is that the check for The problem is that the cldr dependency is called two different things: The brittle fix is just to move the check for cjs before the amd check. |
@jquense, you are saying that webpack is capable of interpreting both CJS and AMD modules simultaneously and it works. On AMD, |
I should probably demonstrate with code. If you
and here is the UMD wrapper globalize defines (this is around: if ( typeof define === "function" && define.amd ) {
define(["cldr","cldr/event"], factory );
} else if ( typeof exports === "object" ) {
module.exports = factory( require( "cldrjs" ) );
} else {
// global one omitted
} By default Webpack understands both module formats so as it parses this file it hits: if it had run: |
And I'm saying that on AMD there's no hard definition for module names. Let me demonstrate with code for you as well https://github.com/jquery/globalize/blob/master/test/config.js#L4 (on AMD you must tell where the modules come from or it's expected there's a file on baseDir with such name). Anyway, can you make a test please? If you rename |
Gotcha, sorry I don't use AMD modules, forgive my ignorance :). Yeah if I change the name it fails on |
We need to understand how to pass an AMD config to webpack or have webpack to simply ignore the AMD definitions. |
So that is easy enough to do as the person webpacking the files. Unfortunately I don't think there is anything module authors can do to define configurations for potential consumers. How does it work with Some context. I'm coming from the React.js community which mostly uses cjs, npm, and webpack to publish and distribute components (both for server/browser). The advantage being consumers of my components will just get all my dependencies installed and webpack/browserify will package them up without a consumer needing to have any knowledge of my dependencies and I don't have to create a bundle to publish I can just keep my package all in modules. |
on second thought tho if you are going to configure the amd stuff anyway then my original solution should work? Reordering the UMD to: if ( typeof exports === "object" ) {
module.exports = factory( require( "cldrjs" ) );
} else if ( typeof define === "function" && define.amd ) {
define(["cldr","cldr/event"], factory );
} else {
// global one omitted
} |
Using AMD, they would need to set it all up themselves AFAIK.
Does reordering work for you locally please? |
Can confirm that it Works on My Machine™ if I reorder the UMD wrapper |
@jquense Could you elaborate on how you got Globalize working with Webpack please? I'm having the exact same issue. |
@JeremyRylan, for now as a workaround, try swapping the AMD and CJS exports of the UMD wrapper (e.g., https://github.com/jquery/globalize/blob/1.0.0/dist/globalize.js#L19-L30), i.e., by placing the CJS in front of the AMD. |
@rxaviers Perfect! That works for me. Thank you very much for the quick response and for the amazing work with Globalize. Do you think this is something that will be addressed in a future release or will we rely on having to modify the source? |
You're welcome. This is definitely something we want to get sorted out. Although, it's important that such solution doesn't break anything else. It would help if you (or @jquense) could share with us a step-by-step procedure (for example, in a gist) to reproduce your use-case. |
@JeremyRylan A less brittle solution you can try is to add this loader to your webpack config { test: /globalize/, loader: 'imports?define=>false' } Which will tell Webpack to ignore AMD imports for modules that match globalize packages (untested). As I say in above posts, this is an ok solution for app developers, but not feasible for building libraries that depend on globalize. For the most simple repo of the problem: https://gist.github.com/jquense/10e3d57e1ce8ae0467da If you are looking for a real life use case you can check out https://github.com/jquense/react-widgets which is a component library I maintain that depends on globalize for date and number pickers Right now to get this to work with
The first is not feasible, as it destroys a lot of the benefits of using modules, and means that users cannot consume my library ala carte. Right now you can do: Number two is not ideal, but for all the reasons I've said earlier. |
@rxaviers Any chance we could see this fixed in the next release of |
@rxaviers Here's your The |
Thanks for your example. I may reply in a week to 2 weeks timeframe. I do hope we can get this sorted out for the next release yes. |
@jquense The custom loader thing worked great for me. Thanks! |
Hi everyone, I've finally spent some time digging into this issue, understood it beeter and I'd like your input. The problem is that webpack finds the AMD definition first and tries to use this instead of the CJS definition. This is not a problem if I understood the two solutions/workarounds that have been raised are:
For the first one, I am not comfortable making changes motivated by its side-effect solving another completely different thing. For the second one, like @jquense said, it's not feasible for building libraries that depend on Globalize. I assume, because it's cumbersome asking developers to include such non-trivial loader. Going further, I gave a try at @unindented's example and I realized a second issue: the inability to handle feeding Globalize on CLDR data. This is, when CLDR data is static (e.g., https://github.com/unindented/globalize-hello-world-webpack/blob/master/main.js#L6-L9) webpack works just fine. Although, when CLDR data is dynamic (and it is in most of the usecases, e.g., These problems suggests a solution should:
var Globalize = require("globalize");
Globalize.formatNumber(Math.PI); Unfortunately, I could not think of any solution that doesn't involve adding configurations to webpack. To minimize the inconvenience though, I thought we could concentrate all in one single GlobalizePlugin. On top of that, a third point, not yet mentioned here, is to allow webpack to optionally generate precompiled formatters and parsers for production (faster and smaller code) instead of including CLDR data and having formatters and parsers to be created dynamically in the client. I've created it, which certainly needs more iterations given your feedback.
The plugin currently lives in https://github.com/rxaviers/globalize-webpack-plugin/tree/b0.0.1 (very bad documented so far). The best I can show for now is a React.js example that uses it: https://github.com/rxaviers/yarsk/tree/react-globalize. It's all work in progress, but I wanted to share fast to receive your early feedback. I invite everyone to join the brainstorm and to contribute to a proper solution. |
@rxaviers Looks great so far! What do you think of building an app that depends on a library that in turn depends on Globalize? |
I will dig into this a bit more when I get some free time. Thanks a ton for really exploring and looking into this. For now here are some quick thoughts.
Changing the order fixes most of the issue, and doesn't seem like it would be a big deal, there is no reason why the AMD defs need to come first in the umd wrapper, and plenty of umd variations order differently. I do totally get the yuck factor of that solution tho. A more robust solution, and a fairly common one, would be to build just commonJS modules for the NPM package. If it makes a difference i'd be happy to help contribute that build step. In terms of the second issue of dynamic json loads, I haven't run into this personally (or haven't realized I did), since I usually just require the json files directly instead of using |
Your contribution is very welcome and I'm open for such change. But, first let's get this better defined. Let's move this discussion into: #467. PS:
Across various jquery projects, we use UMD/returnExports. So, if changes are made upstream, I won't object updating it in here as well. |
Can we go back to the inconsistency of if ( typeof define === "function" && define.amd ) {
define(["cldr","cldr/event"], factory );
} else if ( typeof exports === "object" ) {
module.exports = factory( require( "cldrjs" ) );
} else {
// global one omitted
} This is somewhat related to the cldrjs "export name" discussion: rxaviers/cldrjs#7 Maybe a better solution here is to actually rename |
We could, but that alone would not solve this problem given AMD paths would On Wednesday, August 5, 2015, Jörn Zaefferer [email protected]
+55 (16) 98138-1583, +1 (415) 568-5854, skype: rxaviers |
All, #481 implements an example and it's supposed to fix all problems raised here. Therefore, it's going to be considered as a fix for this issue. It's almost complete and by now one should be able to run the example. It would be great if you could check it and provide your feedback. |
This has been fixed with webpack plugins, which are pretty solid now (Twitter mobile uses it). More details in the demos. |
@rxaviers I am having issues with bower-resolve-webpack-plugin and globalize-plugin to work together. If I install gloablize in node_modules it works fine. But globalize-webpack-plugin doesn't pickup the globalize module from bower_components. Note that all my other modules are loaded from bower_components using bower-resolve-webpack-plugin. bower-resolve-plugin is using existing-directory plugin to process all the stuff. When webpack hits globalize, it ignores that plugin somehow. The following is my configuration.
The following is the error: |
@nishantkagrawal please file an issue against rxaviers/globalize-webpack-plugin preferably with a reduced reproducible demo. |
Hi @rxaviers , I'm currently writing a library that depends on globalize but I have no control over consumers' webpack configs, i.e. globalize-webpack-plugin is not an option. What are my options here? |
FYI, for newer Webpack: {
module: {
rules: [
{ test: /globalize/, parser: { amd: false } }
]
}
} |
congrats on
1.0
! been waiting a long time to use the shiney new globalize :)I have been running into issues using Globalize with webpack ( browserify may suffer similarly).
Investigation shows that this is due to the AMD check running first in the UMD wrapper for the files, since the package has been installed from npm there is no
cldr
package for webpack to find. there is acldrjs
however and if I tell webpack to ignore amd style modules for globalize it works just fine.The problem is that as a library author using Globalize and publishing to npm I can't ask consumers of my module to add a special exemption to there build process in order to get my module to load. I am not sure what the best way to "fix" this is other then building a commonjs (or amd) only version of globalize that gets published to npm, instead of the general
/dist
build that works for both bower and npmThe text was updated successfully, but these errors were encountered: