Skip to content

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

Merged
merged 1 commit into from
Sep 24, 2015
Merged

Build: Add main entry for webpack support; actual value doesn't matter #1600

merged 1 commit into from
Sep 24, 2015

Conversation

jzaefferer
Copy link
Member

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/9574b6ede7287a39741410363745f625fabc1b01

It 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 the main 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.

@scottgonzalez
Copy link
Member

@bebraw thoughts?

@JSteunou
Copy link

@jzaefferer please see http://bugs.jqueryui.com/ticket/14375

Instead of a fake main entry, a real entry pointing to a module (index.js ?) exporting all others ui module could be nice. It might not be used a lot, because that would mean loading all jQuery UI modules while you need just one, but could serve some purpose (testing?)

@bebraw
Copy link

bebraw commented Sep 20, 2015

@scottgonzalez Not really. Setting that main solves a lot of issues.

@jzaefferer
Copy link
Member Author

Instead of a fake main entry, a real entry pointing to a module (index.js ?) exporting all others ui module could be nice. It might not be used a lot, because that would mean loading all jQuery UI modules while you need just one, but could serve some purpose (testing?)

"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 index.js, I think that would do more damage than good. "some purpose (testing?)" is too vague, even for unit or integration tests I don't see how loading the entire library would help.

Setting that main solves a lot of issues.

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 main entry?

@bebraw
Copy link

bebraw commented Sep 21, 2015

@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?

@JSteunou
Copy link

@jzaefferer You read my comment to quickly I suppose.

"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.

It's not an accident if you explicitly wrote require('jquery-ui).

Adding an index.js aside with a main: 'index.js' would be cleaner than a fake main entry pointing to one randomly picked module.

And @bebraw got a point, requiring a specific module return... not the required module.

@JSteunou
Copy link

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?

@jzaefferer
Copy link
Member Author

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.

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?


It's not an accident if you explicitly wrote require('jquery-ui).

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).

Adding an index.js aside with a main: 'index.js' would be cleaner than a fake main entry pointing to one randomly picked module.

I'm not happy with the arbitrary value, as my related question above should indicate. That doesn't make an index.js any better. You still haven't answered my question about the actual usecase for such an index, until then "cleaner" doesn't matter to me.

Why jquery-ui does not follow the same pattern?

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!

@JSteunou
Copy link

If cleaner does not matter to you then go ahead. I have no other arguments than normalization versus random.

@jzaefferer
Copy link
Member Author

I care about "cleaner", but there's a tradeoff here, and encouraging users to build bad packages is worse in my view.

@bebraw
Copy link

bebraw commented Sep 21, 2015

I can't tell what you're testing here. In my own test/demo, this works fine, although with a different path:

Got it. I was looking at npm version instead of GH. Obviously doing npm i jquery/jquery-ui --save fixed it and i can see the expected result for my dialog import.

Can you confirm this is the right approach? Am I missing something?

It seems valid. import dialog from 'jquery-ui/dialog' would look nicer than pointing through 'jquery-ui/ui/widgets but that's for you to decide of course. 👍

@jzaefferer
Copy link
Member Author

It seems valid. import dialog from 'jquery-ui/dialog' would look nicer than pointing through 'jquery-ui/ui/widgets but that's for you to decide of course.

I prefer the shorter format. With requirejs I'd set up a path in my app to achieve that, how can we do that with webpack?

@bebraw
Copy link

bebraw commented Sep 21, 2015

I prefer the shorter format. With requirejs I'd set up a path in my app to achieve that, how can we do that with webpack?

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. 😄

@jzaefferer
Copy link
Member Author

Thanks, that's pretty similar to requirejs then, in that adding the resolve.alias has to happen in userland. Its not without issues, since the path for the CSS files need to change. I came up with this, which I don't like enough to actually put in the demo: https://gist.github.com/jzaefferer/18e15f6b2f125d6d6cfc

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?

@bebraw
Copy link

bebraw commented Sep 22, 2015

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 import _ from 'lodash'). The plugin will transform that to something more specific based on usage. I expect the same idea would work for you but obviously not many people could benefit from that yet. 😄

@jzaefferer
Copy link
Member Author

It's a good idea to document that for those who want to use jQuery UI through Webpack.

I've added that to the readme: https://github.com/jzaefferer/webpack-jquery-ui#using-resolvealias-to-simplify-imports

The same idea applies to JS as the tooling might be able to eliminate dead code. It's difficult to rely on that.

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.

@bebraw
Copy link

bebraw commented Sep 23, 2015

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.

@jzaefferer
Copy link
Member Author

@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?

@Gaurav0
Copy link

Gaurav0 commented Sep 23, 2015

@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.

@jzaefferer
Copy link
Member Author

Okay, thanks. Landing this as-is then.

@jzaefferer jzaefferer merged commit 6308a26 into jquery:master Sep 24, 2015
@jzaefferer jzaefferer deleted the webpack branch September 24, 2015 08:51
@mnpenner
Copy link

mnpenner commented Sep 1, 2016

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).

@scottgonzalez
Copy link
Member

Does jquery-ui follow semver?

Not yet. You should treat our minors as majors.

@mnpenner
Copy link

mnpenner commented Sep 1, 2016

Well... alright then. I'd better fix my package.json then 😝

@TheWitness
Copy link

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

@scottgonzalez
Copy link
Member

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.

@TheWitness
Copy link

TheWitness commented Sep 1, 2016

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.

@jzaefferer
Copy link
Member Author

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.

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.

8 participants