yui / yui3

A library for building richly interactive web applications.
http://yuilibrary.com/
Other
4.12k stars 1.29k forks source link

[core] Support conditions for polyfills #1585

Open caridy opened 10 years ago

caridy commented 10 years ago

The current conditional loading does not support the schema where the conditional module defines whether or not it should be attached, and instead it requires other modules to act as a trigger, which will requires a polyfill to know about all modules that depends on it.

Instead, we want to support two new features:

juandopazo commented 10 years ago

I don't understand. If a module requires a feature, shouldn't it just add it as a dependency?

clarle commented 10 years ago

What's the difference between the existing condition.test and condition.tests?

Would tests just be an array of functions instead of just one?

caridy commented 10 years ago

@clarle tests is at the top level in this proposal, it is NOT within condition since condition is tackling a different use-case.

juandopazo commented 10 years ago

Clarifying:

{
  modules: {
    'some-polyfill': {
      test: function (Y) {
        return Y.UA.ie;
      }
    },
    'my-module': {
      optionalRequires: ['some-polyfill']
    }
  }
}
caridy commented 10 years ago

few more notes:

ericf commented 10 years ago

So in @juandopazo's example, when "my-module" is required/used, the system will:

  1. iterate over its optionalRequires
  2. If an optional requires does not have a test(s), it's skipped.
  3. If it does have a test(s), and any tests fails (returns false), it's skipped.
  4. If all test(s) pass, then it's added to "my-module"'s requires list and loaded.

Is this correct?

juandopazo commented 10 years ago

Considering the big refactor on the way, shouldn't we keep new features to a minimum? We've been doing ok without global tests so far. And tests seems overkill; it´s probably a good idea to avoid more loops in during _attach.

caridy commented 10 years ago

@ericf about your point 4, we should avoid altering requires, this routine should be applied during the getRequires(), which allow us to get the require list and augment it with anything that we think is needed. The problem with requires is that it will alter the amount of ES modules that will be passed into the __imports__.

@juandopazo about the global tests, if you don't have that, you can't add the tests into build.json, unless you change shifter to do the same it does with condition.test. We already have some build processed around global tests that were in place for the already deprecated Y.Features, we could use some of that.

ericf commented 10 years ago

Can we document the reason why we can't use optional for this feature?

caridy commented 10 years ago

current feature called optional is an artifact to control order, not to control requirements. When a module is added to optional it means the module should have precedence in the requirement list, and it should be added to the requirement list only if another module is requiring it or it is explicitly called during the use statement. e.g.:

"cssgrids": {
        "optional": [
            "cssnormalize"
        ],
        "type": "css"
    }

in this example, we are saying that if you do this:

Y.use('cssgrids', 'cssnormalize' ....);

it should attach cssnormalize before cssgrids even though it is not an explicit requirement. Although, if you do this:

Y.use('cssgrids' ....);

then cssnormalize will not be included at all.

In general, I'm not oppose to the use of optional for css, but I do oppose to the use of it for scripts, because it makes the dependencies very fragile.

In the current metas, I see a lot of mis-used optional entries. Let me list a couple of examples:

One:

this is probably just a incomplete clean up, but worth to mention it:

    "history-html5": {
        "optional": [
            "json"
        ],
        "requires": [
            "event-base",
            "history-base",
            "node-base"
        ]
    },

but when you look at the code of history-html5, there is no use of Y.JSON at all. Probably an old feature were we used json to serialize something there.

Two:

this one is a concrete example:

    "router": {
        "optional": [
            "querystring-parse"
        ],
        "requires": [
            "array-extras",
            "base-build",
            "history"
        ]
    },

and when looking into router.js we can see var QS = Y.QueryString, at the very top (https://github.com/yui/yui3/blob/master/src/app/js/router.js#L10), then a condition to fallback in case that doesn't exists:

 _parseQuery: QS && QS.parse ? QS.parse : function (query) {

https://github.com/yui/yui3/blob/master/src/app/js/router.js#L1229

the reality is that we have a granular module system, and by adding querystring-parse-simple to the requires in router will be equivalent.

There are other problems with the optional, like autocomplete-base with autocomplete-sources as optional, while autocomplete-sources requires autocomplete-base, that's just nuts, but I bet it is a remanence of refactors over the years.

ericf commented 10 years ago

@caridy thanks for writing this up for clarity and posterity. What I'm wondering is whether we can use/re-purpose optional for polyfills. It seems like it's a pretty close match, but things in autocomplete make it a bit trickier.

caridy commented 10 years ago

@ericf yes, but my problem with optional is its semantic, it doesn't tell us anything about the feature itself since it is just a adjective, comparing that to requires for example, which is a verb, and explicitly tells what's going on with that configuration option. Also, we have legacy, we don't know how people are using those things.

Ideally, we could kill optional, and create a loader patch/extension that people can put in place if they need optional in its current form, and we move on with our lives.