-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[loader] Add support for optional dependencies #1629
Conversation
// since they should already be available in the global scope. | ||
if (optReqs) { | ||
for (i = 0, length = optReqs.length; i < length; i++) { | ||
m = this.getModule(optReqs[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
might not even exist in the registry, so this might return a falsy value, and the if
that follows should take that in consideration as well.
normally, modules with affinity can be deployed to specific runtimes, and optionalRequires
will facilitate the edge case where a module that is suppose to run in all runtimes (e.g.: server and client) has an optional require on a module that is suppose to run only on a client, and if you have the proper meta generation process per runtime, you don't need to add tests for those modules, just skip it from the undesired runtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
Note: a module with a |
Hey what is the status of this PR, are you planning on merging or is this dead? |
@saw it is not dead, we just hit some roadblocks while doing the review of the code, and the tests. @juandopazo will continue working on this. That being said, you have a patch in |
@ezequiel just helped me find the issue. The sorting function that sorts the list of modules only looks at |
This way _sort() has knowledge of optional requires to sort them as dependencies
First pass at a solution. Please do not merge this. I'm experimenting with using the |
Just to have it on the record, can we write here the reasons why this use case isn´t covered by |
Here is the full writeup about this issue: |
CLA is valid! |
Reproduced the conflict with patterns. Once #1790 is merged this should be good to go. |
…ultiple YUI instances
I'm happy to report this PR is now passing and ready to go. Project building YUI and trying to use this feature would still need Shifter to support the new |
@@ -737,6 +737,12 @@ with any configuration info required for the module. | |||
Y.Env._missed.splice(j, 1); | |||
} | |||
} | |||
|
|||
if (loader && !loader._canBeAttached(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this to support Y.use('optionalModule')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to avoid attaching the module to the Y
when the module has already been included (maybe in a bundle) and its test fails.
It probably deserves a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added!
have their own tests instead of a test associated with this module like | ||
conditional dependencies. This is targeted mostly at polyfills, since | ||
they may not be in the list of requires because they are assumed to be | ||
available in the global scope. **Modules without a test will be ignored**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is misleading. Modules without a test
will be consider valid modules and will be attached, at least that's what I can infer from the implementation.
Optional dependencies try to be consistent with the idea that they're "optional". If the module is available (loaded and without a test or with a passing test) it is used. If it is not available (with a failing test), it is ignored.
LGTM, although I still see some travis failures happening. |
@caridy that was just a Promise [slightly] flaky test. |
Optional dependencies are modules that are not in the
requires
list because they depend on a test passing. Contrary to the currentcondition
option, the test is owned by the optional module itself, not by the required module.Example:
This is WIP. I'm still figuring out what all the edge cases are and this need changes to Shifter to fully integrate it into YUI.