Handle optional dependencies in loadLanguages()#1417
Handle optional dependencies in loadLanguages()#1417Golmote merged 6 commits intoPrismJS:gh-pagesfrom
loadLanguages()#1417Conversation
|
I'll probably be able to review this PR this weekend, just FYI. |
|
Sure, take your time! |
mAAdhaTTah
left a comment
There was a problem hiding this comment.
I think this is a really nice solution to this problem, and I think we can do it without exposing withoutDependencies (documented or otherwise). Let me know what you think!
components/index.js
Outdated
|
|
||
| function loadLanguages(arr) { | ||
| function getOptionallyDependents(mainLanguage) { | ||
| return Object.keys(components.languages).filter(function (language) { |
There was a problem hiding this comment.
We do this a couple times; we could definitely do this once and remove meta and reuse that array. If we're mostly going to be doing iteration, we could just map over the keys and turn them into values, so end up w/ an array of languages. If we want to go a step further, we could tag the objects w/ a loading property and remove the withoutDependencies information and just recurse until you hit a component that's already being loaded.
This would be a bit more stateful but shouldn't be hard to implement and would allow us to hide the implementation details from consumers. I don't really love exposing withoutDependencies, if if it's not technically public API, and that would do away with it nicely.
There was a problem hiding this comment.
I'm not sure I understand your whole idea here.
We could indeed process the languages only once, though. We could build a structure that maps a language to its "peerDependencies" and therefore just access it by key. Something like:
var dependentsMap = null;
function getOptionallyDependents(mainLanguage) {
if (!dependentsMap) {
dependentsMap = {};
/* Build a structure like:
dependentsMap = {
"fortran": ["pure"],
"coffeescript": ["haml", "pug"],
...
}
*/
}
return dependentsMap[mainLanguage] || [];
}Is it what you meant?
components/index.js
Outdated
|
|
||
| require('./prism-' + language); | ||
| arr.forEach(function (language) { | ||
| if (components.languages[language]) { |
There was a problem hiding this comment.
If we change this to:
if (!components.languages[language]) {
console.warn('Language does not exist ' + language);
return;
}...then we can save an indentation. I like early returns :)
components.json
Outdated
| "title": "CSS", | ||
| "option": "default" | ||
| "option": "default", | ||
| "optionalDependencies": "markup" |
components/index.js
Outdated
|
|
||
| // Reload dependents | ||
| var dependents = getOptionallyDependents(language); | ||
| dependents = dependents.filter(function (dependent) { |
components/index.js
Outdated
| return false; | ||
| }); | ||
| if (dependents.length) { | ||
| loadLanguages(dependents, true); |
There was a problem hiding this comment.
Is the purpose of withoutDependents to avoid an infinite loop? Are we sure it recurses through everything? If it is, maybe we don't need it (see above).
There was a problem hiding this comment.
Actually no, it wasn't to prevent an infinite loop. I think an infinite loop can still happen if we have circular dependencies, hopefully this is not the case at this time AFAIK.
It was supposed to be an optimisation, because you need to reload a component so that it takes its updated "peerDependencies" into account, but you don't need to reload the whole "inheritance" tree. There's already a lot of files being required there, so I tried to keep the amount as low as possible.
# Conflicts: # components.js
|
I updated the PR to address your comments. Regarding Let me know what you think. |
This may be fine; the idea is essentially to keep track of what languages we're in the process of loading and avoid reloading them if we hit them again via circular references. Doing it that way would ensure we recurse down all the way down instead of (what looks like?) one level down. But maybe that's not actually an issue? This is certainly a simpler implementation. |
|
Also, I think this PR may be needed by the babel plugin to ensure we load everything in the correct order. |
|
@mAAdhaTTah Since we don't seem to have any circular dependencies at the moment, I would suggest we keep it this way, at least for now. |
# Conflicts: # components.js
|
Yeah, this is good. Just need to fix the conflict in |
# Conflicts: # components.js
* gh-pages: (33 commits) Add double-class specificity hack (#1435) Moved tutorial link to the end of the list Make line-numbers styles more specific Fix mixed content warning Create CNAME Delete CNAME Update documentation for node & webpack usage Handle optional dependencies in `loadLanguages()` (#1417) Add `objectpascal` as an alias to `pascal` (see #1426) Add support for XQuery. Fix #1405 (#1411) Website: Fix Download page not handling multiple dependencies when from Redownload URL JSX: Add support for fragments short syntax. Fix #1421 Support for Template Toolkit 2 (#1418) ASP.NET should require C# Run gulp Move guard into conditional and check for language Don't process language if block language not set JSX: Allow for two levels of nesting inside JSX tags. Fix #1408 Add missing reference to issue in specific test. Powershell: Allow for one level of nesting in expressions inside strings. Fix #1407 ... # Conflicts: # components/prism-kotlin.js
Fixes #1393