-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Build: Add main entry for webpack support; actual value doesn't matter #1600
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
Conversation
@bebraw thoughts? |
@jzaefferer please see http://bugs.jqueryui.com/ticket/14375 Instead of a fake |
@scottgonzalez Not really. Setting that |
"loading all jQuery UI modules" is a giant drawback. When using webpack, you should accidentally end up including the whole library, when you only use one or two modules. Unless there is a good usecase for this kinda of
I find it still odd that the actual value doesn't matter when resolving individual modules inside the library. Shouldn't that work even without the |
@jzaefferer I can see the problem clearer now. Consider an import like this against the current distribution version: import dialog from 'jquery-ui/dialog';
... In Webpack that returns an empty object by default. Looking at the dialog code, I can see it has been design so that it attached the component to a global jQuery. I suppose in context of Webpack we might be able to consume it through something like exports-loader. That said, wouldn't it be nice if the import returned a component directly? How do browserify guys deal with this? |
@jzaefferer You read my comment to quickly I suppose.
It's not an accident if you explicitly wrote Adding an And @bebraw got a point, requiring a specific module return... not the required module. |
Open related question: jquery works very well with browserify / webpack / ... all npm / CJS builder, it is published on it, advice plugins should be too, communicate on it. Why jquery-ui does not follow the same pattern? |
I can't tell what you're testing here. In my own test/demo, this works fine, although with a different path: var autocomplete = require('jquery-ui/ui/widgets/autocomplete'); (it also works with ES6 modules, but I wanted to keep the demo focused on webpack+ui, instead of complicating it further with extra babel-core+babel-loader dependencies) Can you confirm this is the right approach? Am I missing something?
Its not an accident when someone explicitly includes http://code.jquery.com/ui/1.11.4/jquery-ui.js on their page, but its still a bad idea (and might lead to the too common complaint of UI being bloated, when the reference is a bundle of the complete library).
I'm not happy with the arbitrary value, as my related question above should indicate. That doesn't make an
Dealing with a repository that contains a collection of libraries (jQuery UI) is harder than one that contains just a single library (jQuery). Tools like browserify and webpack are still very new compared to how long jQuery UI has been around. That might help explain why we're not there yet. But as this discussion provides: We're working on it! |
If cleaner does not matter to you then go ahead. I have no other arguments than normalization versus random. |
I care about "cleaner", but there's a tradeoff here, and encouraging users to build bad packages is worse in my view. |
Got it. I was looking at npm version instead of GH. Obviously doing
It seems valid. |
I prefer the shorter format. With requirejs I'd set up a |
We can use resolve.alias on webpack side for that. You'll do something like this: { 'jquery-ui': path.join(__dirname, 'node_modules/jquery-ui/ui/widgets') } Not tested but enough to convey the idea. 😄 |
Thanks, that's pretty similar to requirejs then, in that adding the Either way, that doesn't change anything in jQuery UI itself, so I think we can go ahead and land this. Or is there anything else? |
Not that I can think of. It's great you got that alias to work. That CSS bit seems acceptable even though it's not particularly pretty. I don't think there's a neat way around that. It's a good idea to document that for those who want to use jQuery UI through Webpack. One way would be to include whole CSS and let tooling strip the unused rules. Most people aren't likely aware of those tools, though. The same idea applies to JS as the tooling might be able to eliminate dead code. It's difficult to rely on that. There's a Babel plugin for lodash that allows you to use simple |
I've added that to the readme: https://github.com/jzaefferer/webpack-jquery-ui#using-resolvealias-to-simplify-imports
That's an interesting approach, but apparently pretty difficult to pull off generically. Google's Closure Compiler has tried hard to make that work and, as far as I was involved with applying it to jQuery Core, never got anywhere. I think for now encouraging importing of individual modules is our best bet. We can still improve this later, as tooling improves. |
Absolutely. Dead code elimination is a hard problem for sure. I know Uglify has something for that but I'm not sure how advanced it is. JavaScript tends to be a little too flexible at times making it difficult to pull of analysis. |
@Gaurav0 as the owner/maintainer of ember-cli-jquery-ui, could you take a look at this PR, including the discussion? Does this change make things better or worse for you ember package? Is there something else we can or should change? |
@jzaefferer I currently use bower, not webpack. I don't currently use the npm package at all. I will change to using npm v3 if and when ember-cli does. |
Okay, thanks. Landing this as-is then. |
Fixes #14375 Closes gh-1600
Does jquery-ui follow semver? This is a breaking change. My app no longer works with jquery-ui 1.12. I know you guys are recommending I include every widget individually, but in a million-line app it's kind of hard to figure out what's in use, and what's not. I think the index.js file should be kept, even if it only contains a list of the widgets from the previous release and is no longer maintained. People who are concerned about saving bytes can include each file individually, and those of us who want it to just work can continue to use the bundle. I was about to mention "tree-shaking", but I suspect it won't work here because the jQuery object is modified (i.e. widgets aren't side-effect free). |
Not yet. You should treat our minors as majors. |
Well... alright then. I'd better fix my package.json then 😝 |
Scott, I don't make a habit of trolling comments, but I would like to see a 1.15 that only supports handling/resolving the exceptions generated by jQuery migrate and jQuery 3.1. I was having issues with jQuery 2.x with jQueryUI 1.14, that are now resolved with jQuery 3.1. Downside, is that I'm having to use the jQuery Migrate plugin. Not a good position to be in. That being said, time is not free, and is always fleeting. So, if that did not happen, I am getting what I'm paying for. I would offer to volunteer, but my employer does not make that easy. TheWitness |
I'm honestly not sure what you're referring to as none of the versions of jQuery UI you've mentioned exist. You shouldn't need jQuery Migrate to use jQuery UI 1.12 with jQuery 3.1. |
Yea, my bad ... 1.14 == 1.11.4. I'm dyslexic, which is just above brain dead according to my wife. Right now, 1.12 broke a bunch of things. That's why I asked. |
I think we could help you better if you would create a ticket at https://bugs.jqueryui.com/newticket (login with GitHub!) and describe your issue there. |
Fixes #14375
Very simple change that probably needs a lot of explanation. To show that this actually works, I've created a proper demo: https://github.com/jzaefferer/webpack-jquery-ui
That was inspired by this demo, which contained a lot of unrelated things: https://github.com/bebraw/jquery-ui-webpack-demo
Which in turn was using this
index.js
(default when"main"
property isn't specified): https://github.com/bebraw/jquery-ui/commit/9574b6ede7287a39741410363745f625fabc1b01It turned out that using an index file with a list of all components, besides the annoying maintenance, is also bad for webpack builds, since it ends up including everything listed in that file. On the other hand, including specific components, like in my demo, only includes that component and its dependencies.
Without an
index.js
or themain
property, the webpack build will fail. Since the content or value of the file or property doesn't matter, we should consider reporting this as a webpack bug (instead of considering it a feature). Would like to hear some opinions on that.As for publishing on npm, I think we can just start doing that with 1.12. At least for webpack I don't see any need to change anything else. It likely won't work with browserify out of the box, since it doesn't resolve the AMD dependencies, but that shouldn't be our problem; an additional transform or other plugin should support that, like webpack does by default.