Skip to content
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

Standardized dependency logic implementation #1998

Merged
merged 31 commits into from
Dec 16, 2019

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 24, 2019

This is my first take to implement a standardized dependency logic to resolve #1992.
Edit: This now also resolves #1427.

To keep the interface simple, it exports just one function: getLoad (better names welcome).
This function takes a components map (basically components.json), a list of components to load, and a list of already loaded components and returns two things:

  1. The set of components which have to be loaded.
  2. A function to load the components in order. This function supports both synchronous and asynchronous loading.

This is the first draft and by no means finished.

I also renamed peerDependencies to modify. This is a more accurate description as npm peer dependencies have a different meaning from Prism peer dependencies.

There is still one major problem which is that I don't know how to make this logic available to the website, the loadLanguage function, the Autoloader, and externals.


There are a few changes I made to components.json:

I also renamed peerDependencies to modify. This is a more accurate description as npm peer dependencies have a different meaning from Prism peer dependencies.

The meaning of require changed. Right now, every require is an implicit modify. This is a bit of a problem because it significantly increases the number of reloads as every dependency of a component to load has to be reloaded and every loaded component depending on a component to reload has to be reloaded as well.
By changing the meaning of require to: Load this component before me, I won't change it. The number of reloads decreases.

There are now 3 types of dependencies:

  1. require: As mentioned above it mean: Load this component before me. I won't change it.
    (Classic dependencies. Readonly.)
  2. after: Load this component before me if it's loaded at all. I won't change it.
    (Optional dependencies. Readonly.)
  3. modify: Load this component before me if it's loaded at all. I will modify it.
    (Optional dependencies. Read/Write.)

The old behavior of require can be achieved by using both a require and modify dependency for the same component.


Now a few details about the loading process:

getLoad takes two component sets: load (components to load) and loaded (components which are already loaded).
It generally tries to minimize the number of components which have to be (re)loaded but all components in load are guaranteed to be loaded. Only the dependencies of the components to load might not appear in the final set of components to load. I.e. If we were to load JavaScript and had C like loaded already, we don't need to reload C like.

@mAAdhaTTah
Copy link
Member

I'm going to take a deeper review of this later, but first and foremost: we're gonna need tests for this 😄

@RunDevelopment
Copy link
Member Author

I added a few tests for basic functionality which should cover every feature of getLoad.
The only things I didn't test are the series and parallel functions which are kind of hard to test and should behave correctly if the general load order is correct.

@RunDevelopment
Copy link
Member Author

getLoad now supports aliases.
Aliases in the load and loaded will now be resolved. E.g. js will be resolved to javascript and html to markup.

This means that getLoad(['html', 'svg', 'js']) will now be equivalent to getLoad(['markup', 'markup', 'javascript']).

When merged, this will take care of #1427.

Dependency aliases are not supported meaning that you e.g. can't require js for javascript. I don't think alias support for dependencies is necessary because we just don't use aliases in dependencies.

tests/dependency-test.js Outdated Show resolved Hide resolved
@mAAdhaTTah
Copy link
Member

I dropped this into the Babel plugin and it works! Some minor adjustments but should be good.

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Sep 3, 2019

minor adjustments

Well, since this update is also going to change components.json, we are going to have to change everything related to dependencies. This necessarily includes:

  • Download page
  • Testing suite
  • loadLanguages

The Autoloader can come later but these 3 have to be updated to use the new dependency logic aside from the Babel plugin.

The only reason I haven't done this yet is that I don't want to resolve any merge conflicts in components.json.

@mAAdhaTTah
Copy link
Member

Oh I see. Ok, I need to spend some time thinking about this then.

@mAAdhaTTah
Copy link
Member

I'm a bit concerned that changing the structure & meaning of components.json is a breaking change though.

@RunDevelopment
Copy link
Member Author

Oh yes, this definitely is a breaking change for people who the dependencies from components.json but that is intentional as the current meaning and usage of require and peerDependencies is suboptimal. Both dependencies cause more reloads than necessary (not a huuuge issue but still) and for peerDependencies the name doesn't even describe their purpose (see #1490 and npm peer dependencies).

I always intended this to be a breaking change.

That being said, most users probably won't even notice this because who uses components.json directly beside us?

@mAAdhaTTah
Copy link
Member

Which PRs do you want to get landed ahead of this one?

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Sep 3, 2019

#1944 would be great.
Edit: All other PRs which I expect to be merged in the next couple of days don't modify components.json and can therefore come in whatever order.

dependencies.js Outdated Show resolved Hide resolved
dependencies.js Outdated Show resolved Hide resolved
dependencies.js Outdated Show resolved Hide resolved
dependencies.js Outdated Show resolved Hide resolved
@mAAdhaTTah mAAdhaTTah added this to the 1.18.0 milestone Oct 25, 2019
@mAAdhaTTah
Copy link
Member

@RunDevelopment I tagged this on a new milestone, 1.18.0. If there are other PRs you want to get into the next release, tag them in the milestone as well. I'm going to get back to reviewing this in a bit, but I'll want to get this released as soon as it's ready so I wanna start thinking about what else we want to land there before we do.

@RunDevelopment
Copy link
Member Author

Nice. I'll tag my languages and plugins.

@mAAdhaTTah
Copy link
Member

Let's just bikeshed those last two nits and get this in. I think the API is good, if we get bugs, we'll address them when they get reported.

@mAAdhaTTah
Copy link
Member

mAAdhaTTah commented Dec 15, 2019

And we're ok with the renaming of these properties not being "breaking" from an API perspective? Is this considered a public API? Will we see any real world impact of this? I think I'm ok with it, personally, but I just want to make sure we talk about it.

@RunDevelopment
Copy link
Member Author

From my experience, most people don't even know our languages have dependencies and just assume that every language file is a self-contained package. (I believe that I remember a library that didn't even know that require dependencies are a thing... and most libraries that did know require, didn't know what modify is.)

So I can't say that I feel too bad about changing a detail almost nobody seems to be aware of. And those that are aware of dependencies usually only know our require which didn't change much. Old dependency loaders that only consider require will still work (see Autoloader).

So I don't think that we'll have any negative impact (but only because we don't really tell people how dependencies work). And those that are affected can switch to the new API and (hopefully) be safe for the future.

That being said, I consider dependencies in their current form mostly non-public. The only thing which is vaguely explained is require which didn't change too much. But after this PR we definitely should make this part of the public API and add corresponding documentation.

@mAAdhaTTah
Copy link
Member

That explanation works well for me, although I think we should decide instead that components.js{on} are not considered public but are solely to be used by our internal tools.

@RunDevelopment
Copy link
Member Author

we should decide instead that components.js{on} are not considered public but are solely to be used by our internal tools.

Yes but the whole getLoader should be public API.
I agree that the implementation of dependencies should be just that: an implementation detail. But I still think that we should document how our dependency system works.

@RunDevelopment RunDevelopment merged commit 7a4a0c7 into PrismJS:master Dec 16, 2019
@RunDevelopment RunDevelopment deleted the deps-update branch December 16, 2019 11:49
@RunDevelopment
Copy link
Member Author

@mAAdhaTTah With that 1.18.0 should be ready. I'll finish the changelog.

@mAAdhaTTah
Copy link
Member

@RunDevelopment Are you working on the changelog currently? Once that's in I can do a publish. I'm leaving for vacation Friday, so I'm hoping to get this out before then.

@RunDevelopment
Copy link
Member Author

Yes, sorry. For some reason the changelog script returned an error, so I saw that and stopped for the day. I'll do it now.

@mAAdhaTTah
Copy link
Member

@RunDevelopment No need to apologize, just wanted to check in case I needed to pick that up myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize dependency logic loadLanguages() should work with aliases
2 participants