RaveJS / rave

Zero-configuration application bootstrap and development
278 stars 8 forks source link

Create rave-common-overrides repo to fix broken packages #43

Closed unscriptable closed 10 years ago

unscriptable commented 10 years ago

This repo should contain a bower.json and package.json containing an overrides property. We invented the not-yet-documented overrides property for bower.json or package.json to allow app devs to fix metadata issues.

However, we ignore the overrides feature in third-party packages. We would have to allow third-party overrides somehow. I'll create another issue for that.

The repo should also contain a README that makes contributions/PRs very inviting and clear.

unscriptable commented 10 years ago

See #44.

KidkArolis commented 10 years ago

Yes, I think that would make bower much more useful that it currently is.

I've once started doing something similar (but gave up pretty quickly - npm is the answer imho) - https://github.com/bower-ext/components. I was planning to wrap the bower-cli to extend the bower.json's of the installed packages with the bower.json from the overrides repo.

unscriptable commented 10 years ago

Interesting. "shim" and "package".

KidkArolis commented 10 years ago

yeah.. zero config on the user side that way + better support for old school libs (which has been less of an issue recently)

briancavalier commented 10 years ago

Over in #44, I wrote:

Yeah, I really hope it would shrink to zero in the long term.

Coupla concerns I just thought of:

  1. Timing of fixes and updates: It's possible that an overridden library gets fixed, but the overrides-of-shame are not updated. That could break that package for everyone who upgrades ... hopefully people would start yelling and we'd be able to fix (ie remove its overrides) it quickly.
  2. Versioning: Say version 1 of lib X needs to be overridden, but version 2 doesn't. How would we handle that?
unscriptable commented 10 years ago

Versioning is a major problem. I really don't want to load a semver package into the browser. :(

KidkArolis commented 10 years ago

I think we shouldn't worry too much about versioning. I would expect an override to just work 99% of the time - that bit of the API (main, moduleType, etc.) of any given library doesn't change that often. Worst case - per project overrides could be used.

KidkArolis commented 10 years ago

An alternative to pulling in semver would be to list every version of any bower package explicitly in the overrides..? Doesn't sound great actually..

unscriptable commented 10 years ago

The versioning problem arises when folks do this:

npm install --save angular@1.3.0

Still wondering if this is going to be a huge problem or not. Perhaps you're right @KidkArolis. The moduleType and main don't change very often. As long as we can convince the package authors to fix their libs quickly, this will all be moot.

briancavalier commented 10 years ago

I would expect an override to just work 99% of the time

Yeah, that's probably true. And maybe the only case where it's a problem is when main or moduleType change in a particular lib release in a way that makes them inconsistent with what's in our overrides ... eg angular releases 1.3 and sets main to something different than what's in our overrides.

It does seem like the cases where this will cause problems are far fewer than the cases where it will solve problems :) If, in those rare problem cases, the dev can workaround using a local override, then I think this will all work well.

unscriptable commented 10 years ago

Ok, after a lengthy discussion with @briancavalier (thx Bri!), I think this is an interesting proposal:

  1. Allow "overrides" from the application-level metadata files only.
  2. Create a new, but very similar, metadata property "missing" that supplies missing values. The "missing" property can exist in any metadata file in any package. The values described in "missing" are only applied when values are missing from their associated packages.
  3. We log all of the applied "overrides" and "missing" properties that were encountered when debugging (data-debug attribute).

This allows devs to take control from the application-level metadata, but also allows third-parties to do certain types of "repair work". Specifically, they can provide some metadata until package authors update their bower.json files.

Alternatively, we could allow "overrides" from anywhere, too. As long as we're logging loudly while debugging, we have alerted the app dev where potential problems may lie. Allowing "overrides" from anywhere fixes more broken packages (such as angular on npm, iirc).

Thoughts on any of this?

Specifically, should we allow "overrides" from anywhere?

unscriptable commented 10 years ago

Early attempt is here: https://github.com/RaveJS/rave-overrides

rave-overrides or rave-common-overrides or rave-temporary-overrides or ????

unscriptable commented 10 years ago

@KidkArolis your previous suggestion, "What about fetching the main module and detecting the module type from the code?", is the only reasonable way, atm. We'll have to make this work somehow.

Here's why: Of all the AMD packages on bower, less than 1% include moduleType. The overrides lists would have to be huge in order to make the rave development process reasonably sane for users.

We still need the overrides/missing features, though, to fix packages such as angular via npm and modernizr.

teehemkay commented 10 years ago

What about node packages whose moduleType is actually amd or global (i.e. NOT node)?

I just encounter the case with jQuery File Upload (JFU).

I'm sing npm as my package manager (bower has too many failures for me...)

Interestingly, the JS files in that module seem to only support amd or global as a moduleType.

When RaveJS loads JFU, there is an error message that seem to indicate that jQuery is undefined within that module. My guess is that it's because the line that define the module and specifies jQuery as a dependency never executed.

I added the following override for JFU in my package.json that does resolve the problem for me.

  "rave": {
    "overrides": {
      "blueimp-file-upload": {
        "moduleType": [ "amd" ]
      },
    }
  }

Dunno if this occurs often enough that it warrants a special treatment (e.g. auto-sniffing) other than a manual override though.

KidkArolis commented 10 years ago

I personally think that all js modules in npm should conform to CJS as much as possible - that's what makes npm work so well. So JFU on npm should be fixed to use CJS..

On Wed, Jun 25, 2014 at 1:42 PM, tmk notifications@github.com wrote:

What about node packages whose moduleType is actually amd or global (i.e. NOT node)?

I just encounter the case with jQuery File Upload (JFU) https://github.com/blueimp/jQuery-File-Upload/.

I'm sing npm as my package manager (bower has too many failures for me...)

Interestingly, the JS files in that module seem to only support amd or global https://github.com/blueimp/jQuery-File-Upload/blob/master/js/jquery.iframe-transport.js#L14-L22 as a moduleType.

When RaveJS loads JFU, there is an error message that seem to indicate that jQuery is undefined within that module. My guess is that it's because the line https://github.com/blueimp/jQuery-File-Upload/blob/master/js/jquery.iframe-transport.js#L18 that define the module and specifies jQuery as a dependency never executed.

I added the following override for JFU in my package.json that does resolve the problem for me.

"rave": { "overrides": { "blueimp-file-upload": { "moduleType": [ "amd" ] }, } }

Dunno if this occurs often enough that it warrants a special treatment (e.g. auto-sniffing) other than a manual override though.

— Reply to this email directly or view it on GitHub https://github.com/RaveJS/rave/issues/43#issuecomment-47095378.

unscriptable commented 10 years ago

Auto-sniffing for node/cjs-formatted modules seems a lot trickier than auto-sniffing for AMD. Is just searching for require, module, or exports enough? Searching for exports.\b or module.exports\s*= might be a bit better. Also, since AMD can have those same signatures, we'd have to eliminate AMD as a possibility, as well.

Of course, common UMD code like this would break all reasonable efforts to pattern-match.

tl;dr: I'd rather avoid auto-sniffing for cjs modules.

I personally think that all js modules in npm should conform to CJS

Interestingly, that's the general consensus in the node/npm world. However, unless npm enforces cjs-formatted modules, they're going to get more packages like JFU.

Glancing at JFU's code, I'm finding lots of other problems. That repo is a mess. Still, JFU might be a good candidate for inclusion in the rave-common-overrides repo.

teehemkay commented 10 years ago

I personally think that all js modules in npm should conform to CJS

Interestingly, that's the general consensus in the node/npm world.

Since npm makes for a fine generic package manager maybe a moduleType property ("explicit is better than implicit") would be useful?

Glancing at JFU's code, I'm finding lots of other problems. That repo is a mess. Still, JFU might be a good candidate for inclusion in the rave-common-overrides repo.

+1 for inclusion in rave-common-overrides repo

But yep, it does indeed have lots of problem. I had to fix some problems related to module IDs for example.

KidkArolis commented 10 years ago

I agree that npm is a fine generic package manager. But I think it's generic when it comes to languages, e.g. you can put css, c++, etc. modules there.

When it comes to JavaScript - moduleType is a bad .. pattern. There are many more library consumers than library authors, which is why I think the library authors should put in an effort to conform to CJS. Then - everything just works for everyone out of the box. What is the advantage of having some modules have moduleType: ["AMD"] and then having to adapt all tools to consume that? Even if that was done - the module types are not actually semantically compatible in the first place, e.g. AMD has async requires([]) and loader!plugins and CJS doesn't have an equivalent. And if you look at the overlapping functionality of CJS and AMD - they're equivalent - so there isn't really any point in mixing both in one package manager.

When it comes to requiring css and html and other such things - I think that is underspecified atm and yes - mixed approaches are being used in npm, e.g. some modules rely on browserify transforms, some modules rely on atomify to build the css. But that's a different story - JFU, for example, is JavaScript only, so why complicate things and bring in more branching into npm..

Having said all that, it's not clear to me how node and npm will handle ES6 modules, perhaps a moduleType of sorts will be introduced then, but that will be cause we want to migrate all modules into ES6, which is actually also gonna be beneficial to all browser devs currently using AMD.

teehemkay commented 10 years ago

I agree that npm is a fine generic package manager. But I think it's generic when it comes to languages, e.g. you can put css, c++, etc. modules there.

Don't you think it may well also be generic within JavaScript land when it comes to supporting different kind of module system (which sadly is the current situation and will be for quite sometimes it seems)?

I can't speak for the author of JFU,

But I was wondering if the reason why JFU is not a CJS module is simply because it is clearly a Client Side JavaScript module and maybe there is this perception that CSJ modules are only for Server Side JavaScript (which is clearly not true even though not all AMD loader may support loading CJS modules).

Now, this is the first time I've encountered a module packaged with node but that only supports AMD and globals so I have no idea how common this practice is.

But I can imagine people writing AMD only modules and using npm as a package manager. (in my experience, npm has been much more robust than bower)

But that's a different story - JFU, for example, is JavaScript only, so why complicate things and bring in more branching into npm..

I'm not sure I understand why this would "bring in more branching into npm" :)? There are other tools that put additional metadata in package.json (e.g. grunt, jshint) and that seems to be an accepted practice.

Having said all that, it's not clear to me how node and npm will handle ES6 modules, perhaps a moduleType of sorts will be introduced then,

Exactly and frankly that was the main reason why I even mentioned the moduleType idea.

but that will be cause we want to migrate all modules into ES6, which is actually also gonna be beneficial to all browser devs currently using AMD.

Aren't there other benefits even now, of being explicit about the module type?

For example further thinking about this I had these questions.

Does RaveJS wraps CJS module into a define()? How does that work in the case of a module that supports both AMD and CJS? Wouldn't being explicit about the module type help in this case by suppressing a check for AMDness?

All that said, I see your point and I agree with you that a standardization toward CJS (or even better ES6) in npm would be simpler and better. I'm just wondering if in the current messy world, being explicit about the module type wouldn't be helpful. I don't know :).

unscriptable commented 10 years ago

Too many things to +1, above. :)

Anyways, the npm folks need to figure out how to support ES6 alongside CJS-formatted modules. I know they're resistant to moduleType, but perhaps a boolean flag indicating ES6? We'll see what they choose.

@teehemkay: After looking a bit more, I'm concerned about the quality of the JFU repo. I don't want to turn our rave-common-overrides repo into a dumping ground of monkey-patches for broken packages. I'm thinking rave-common-overrides should have metadata fixes only. We either need to convince the author to fix it or somebody needs to create a dedicated rave "patch extension" for it: a rave extension that is dedicated to monkey-patching the JFU repo.

One of the things that must happen is that the JFU package (or a rave-patched version of it) needs to be separated into several framework-specific packages. For instance, I see js files that reference "angular" packages, but there are no "angular" dependencies in the bower.json or package.json. This is obviously broken.

teehemkay commented 10 years ago

I don't want to turn our rave-common-overrides repo into a dumping ground of monkey-patches for broken packages

I totally agree :)

We either need to convince the author to fix it or somebody needs to create a dedicated rave "patch extension" for it: a rave extension that is dedicated to monkey-patching the JFU repo.

I'll try and look into that possibility next week-end. I'm really interested in better understanding how rave works. Is there some kind of "breadcrumb trail" documentation or discussion of how rave works?

unscriptable commented 10 years ago

The functionality of rave's crawler no longer supports the concept of a third-party overrides mechanism such as a rave-overrides repo. However, so far, I have not found a need for one. The new auto-detection mechanism and heuristics seem to work very well -- EVEN FOR BOWER @_@.