RaveJS / rave

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

Apply rave.missing as default overrides from all rave extensions. See #43. #49

Closed unscriptable closed 10 years ago

unscriptable commented 10 years ago

Stop expecting moduleType to be an array. Stop supplying a moduleType array, so it can be provided if missing/undefined.

unscriptable commented 10 years ago

This was quite easy to do. So far, it only applies the "missing" overrides.

Still wondering if it's the right thing. I guess if I add code in rave/debug to make it clear that overrides have been applied, it's not going to cause chaos.

unscriptable commented 10 years ago

I added a debug message per override/missing applied. However, it may be a bit much to log one line per override/missing:

Package `legacy-experiments` overrode metadata properties of package `curl`.
Package `rave-overrides` provided default metadata for missing properties of package `json3`. 
Package `rave-overrides` provided default metadata for missing properties of package `jquery`. 
Package `rave-overrides` provided default metadata for missing properties of package `underscore`. 
Package `rave-overrides` provided default metadata for missing properties of package `modernizr`. 
Package `rave-overrides` provided default metadata for missing properties of package `bootstrap`. 
Package `rave-overrides` provided default metadata for missing properties of package `fastclick`. 

I'm guessing the typical list of overrides won't get too much larger than this?

Should the message be more of a warning? Is there a better wording? Any other thoughts?

unscriptable commented 10 years ago

Perhaps put the overridden package on the left where it can be seen better:

Package `json3` had missing metadata properties provided by package `rave-overrides`. 
unscriptable commented 10 years ago

I realized something important: these overrides and missing defaults are applied to rave's version of the metadata. Rave keeps a copy of the metadata bits that are important, such as name, version, main, moduleType, and adds some of its own, such as location and metaType.

It's an abstraction leak (and a developer surprise) that the dev or package author are overriding rave's curated version of the metadata.

For instance, if a developer wants to override a browser property in package.json, she cannot do it. Also, it might be a surprise to find out she should use the location property instead of the CommonJS-standard (but never implemented) directories.lib property to change the location of a package's modules.

I'd like to move forward with the current solution and add a github issue to allow overrides at the correct level. Any objections?

fabricematrat commented 10 years ago

Can't we have a full list with the spec ? This will avoid the one by one fix. Just a suggestion On Jun 27, 2014 8:41 PM, "John Hann" notifications@github.com wrote:

I realized something important: these overrides and missing defaults are applied to rave's version of the metadata. Rave keeps a copy of the metadata bits that are important, such as name, version, main, moduleType, and adds some of its own, such as location and metaType.

It's an abstraction leak (and a developer surprise) that the dev or package author are overriding rave's curated version of the metadata.

For instance, if a developer wants to override a browser property in package.json, she cannot do it. Also, it might be a surprise to find out she should use the location property instead of the CommonJS-standard (but never implemented) directories.lib property to change the location of a package's modules.

I'd like to move forward with the current solution and add a github issue to allow overrides at the correct level. Any objections?

— Reply to this email directly or view it on GitHub https://github.com/RaveJS/rave/pull/49#issuecomment-47385603.

fabricematrat commented 10 years ago

Shall we create rave.missing for every popular modules and some pressure to fix them ? On Jun 27, 2014 8:41 PM, "John Hann" notifications@github.com wrote:

I realized something important: these overrides and missing defaults are applied to rave's version of the metadata. Rave keeps a copy of the metadata bits that are important, such as name, version, main, moduleType, and adds some of its own, such as location and metaType.

It's an abstraction leak (and a developer surprise) that the dev or package author are overriding rave's curated version of the metadata.

For instance, if a developer wants to override a browser property in package.json, she cannot do it. Also, it might be a surprise to find out she should use the location property instead of the CommonJS-standard (but never implemented) directories.lib property to change the location of a package's modules.

I'd like to move forward with the current solution and add a github issue to allow overrides at the correct level. Any objections?

— Reply to this email directly or view it on GitHub https://github.com/RaveJS/rave/pull/49#issuecomment-47385603.

unscriptable commented 10 years ago

Shall we create rave.missing for every popular modules and some pressure to fix them ?

Hey @fabricematrat, that is what we aim to do with #43.

Can't we have a full list with the spec ?

Perhaps documentation is a possible answer. We just ensure that devs know that they're overriding rave's internal metadata, not bower.json or package.json metadata (which are similar, but not the same).

fabricematrat commented 10 years ago

I understand but there is the feeling of people not feeding properly a conf file + the strange impression that thing are complicated for no reason. How someone will know he needs to use an override or not ? On Jun 27, 2014 9:06 PM, "John Hann" notifications@github.com wrote:

Shall we create rave.missing for every popular modules and some pressure to fix them ?

Hey @fabricematrat https://github.com/fabricematrat, that is what we aim to do with #43 https://github.com/RaveJS/rave/issues/43.

Can't we have a full list with the spec ?

Perhaps documentation is a possible answer. We just ensure that devs know that they're overriding rave's internal metadata, not bower.json or package.json metadata (which are similar, but not the same).

— Reply to this email directly or view it on GitHub https://github.com/RaveJS/rave/pull/49#issuecomment-47388546.

unscriptable commented 10 years ago

Ok. Gonna merge this for now. Created a new issue (#50) to do it right. Also, we need to finish #41!