Common libraries in src/lib #625
Conversation
- Libraries are under src/lib/ - Added to build.js as usual, before plugins. - See src/lib/README.md for details gc library implements a "garbage collector" library, which allows both the core and plugins to store elements and listeners to a list, and when impress().lib.gc.teardown() is called, to have all of them removed from the DOM. It also allows plugins to register their own callback functions, which are called at teardown. Commentary: This work is based on copying the src/lib/gc.js from impressionist. While it was useful, it turns out on the impress.js side there was much more a need to reset attributes rather than delete elements. For now, this means lots of plugins do this via their own lib.gc.addCallback() functions. Probably it would be nicer to add some generic lib.gc.resetAttributes() functionality for this particular case. I'll return to this in a future patch. extras/ are not supported for impress().tear(). What can I say, they're extras. Maybe in the future I'll support them, for now I can live without.
This facilitates them being used from plugins as well as core impress.js.
There was a problem hiding this comment.
I've made a couple of comments in regards the library and the GC utility. Perhaps it's missing the use cases where that would be applicable?
I see that there are a lot of changes and no testing. Can we at least build test coverage for the new code if we don't think it's worth to spend time building coverage and refactoring the existing ones?
Perhaps we can create a small PR adding just a subset of the functionality in impress.js that is needed for one plugin instead of creating a big one with a lot of assumptions of what's needed?
That will allow a more efficient review and reduce the chance that we'll find ourselves with a lot of more code we can't change easily in the future.
| next: empty | ||
| next: empty, | ||
| tear: empty, | ||
| lib: {} |
There was a problem hiding this comment.
I'm not sure if it's wise exposing all the internals of impress.js for consumers unless there's a reason to do so. The more we expose, the more we're required to support and the hardest it is to change impress.js.
There was a problem hiding this comment.
lib (together with other API methods) are exposed to plugins, therefore ARE our public API.
Libs are not only meant for internal use, but to anyone who wants to build a plugin. So in such case they become a part of impress.js API.
Yes, this means we need to support these and maintain them. Theoretically it will make impress.js 'harder' to change later (to keep backwards compatibility), but it's a part of the process of making impress.js more modular and extensible. We cannot change impress.js if we don't change something ;)
There was a problem hiding this comment.
Cool then. Perhaps in the future, we'll be able to see the patterns clearer and that will allow us to make better decisions on what to expose and what not.
There was a problem hiding this comment.
tear() is a "public" API method, partner to init(). I will add it to DOCUMENTATION.md in this PR, thanks for pointing that out.
lib is "internal" in the sense that it is intended for plugins. But as you say, anybody could write plugins. My own idea here was that:
- As for documentation, README files and source code comments are sufficient. (Of course, we could adopt something like jsdoc if we want.)
- As for public/private, I was thinking since we are an open source project, that the library API have similar guarantee as the Linux kernel: If we need to change them we will. If you don't want your plugin to break, you can contribute it to the upstream repo - with tests ;-) - and it won't break. If you have your own 3rd party plugin, make sure to check version of impress.js and hopefully the API only breaks between versions, not within.
There was a problem hiding this comment.
The second point is interesting, however, if we restrict the scope by default we'll avoid unintended breakage and issue reports that will surely come. We can experiment with that approach, though, and see if it works for impress.js. Unfortunately, Github itself doesn't help when we try to apply Linux principles such as Linux-like forking mechanisms...
| var type = eventListenerList[ i ].type; | ||
| var listener = eventListenerList[ i ].listener; | ||
| target.removeEventListener( type, listener ); | ||
| } |
There was a problem hiding this comment.
Can we use .forEach instead of imperative loop?
There was a problem hiding this comment.
'js/impress.js' is a generated file (built from src/impress.js and other source files), so please comment on source files rather then generated file.
There was a problem hiding this comment.
And I don't mind it being a regular for loop instead of .forEach.
Especially that in cases like calling callbacks it's used to call them in specific order.
There was a problem hiding this comment.
'js/impress.js' is a generated file (built from src/impress.js and other source files), so please comment on source files rather then generated file.
Oh right, didn't realize that lol.
Especially that in cases like calling callbacks it's used to call them in specific order.
If you are referring to the callback being executed in the inverse order, it's just a matter of calling:
callbackList.reverse().forEach;That can simplify the code and reduce the vectors of failure (less moving parts and variables like the index assignment and manual increment)
There was a problem hiding this comment.
reverse is reversing array in place which may not always expected result as it changes the array for later uses, which by itself is a 'moving part' to remember about.
I agree that .forEach makes code more concise, but it also introduces additional function, which adds a bit of complexity by itself:
elemenList.forEach(function(element) {
element.parentElement.removeChild(element);
});
So it really has pros and cons and I'm fine with leaving it to @henrikingo preference and would certainly not block merging this PR on it.
There was a problem hiding this comment.
Code base has avoided forEach due to the version of buildify that was used wouldn't support it. Recently I actually disabled the minify part of buildify, so this is no longer an issue, but it is the reason existing code doesn't use forEach.
(And as explained elsewhere, these kind of code style comments are out of scope as a general category. The goal of these PRs is to merge code that already exists in my repo.)
| steps[ i ].style.position = ""; | ||
| steps[ i ].style.transform = ""; | ||
| steps[ i ].style[ "transform-style" ] = ""; | ||
| } |
There was a problem hiding this comment.
See this comment for the discussion about this.
There was a problem hiding this comment.
This is actually a bit more complicated case, as querySelectorAll doesn't return an array but NodeList that just recently got support for forEach, but only in Firefox and Chrome. So for IE it still needs to be transformed into array to use forEach.
There was a problem hiding this comment.
Nothing prevents one from doing [].slice.call(document.querySelectorAll).forEach or using the built in arrayify(document.querySelectorAll).forEach, it's a pretty common JavaScript pattern to workaround that problem.
However you've just stated that you're ok with this style, so I guess that's fine then.
There was a problem hiding this comment.
Of course [].slice.call(document.querySelectorAll).forEach can be used, the point is - is it really simpler that a regular for loop?
But, let's leave these for loops aside as it's not getting us anywhere :)
There was a problem hiding this comment.
Yeah sure, there are some arguments in regarding creating pure functions over imperative loops that allow an easier extension.
Probably a conversation for another time. Perhaps a future PR since it's an easy change.
src/lib/README.md
Outdated
| var instanceVar = {}; | ||
|
|
||
| // LIBRARY FUNCTIONS | ||
| var libararyFunction1 = function () { |
| The `src/lib/*.js` files contain library functions. The main difference to plugins is that: | ||
|
|
||
| 1. Libraries are closer to the impress.js core than plugins (arguably a subjective metric) | ||
| 2. Libraries are common utility functions used by many plugins |
There was a problem hiding this comment.
Is a library API really necessary to be provided by impress.js? It looks like this is moving away of the purpose of impress.js which is serving as an engine for presentations and being more of a container for utilities.
What's exactly the use case?
There was a problem hiding this comment.
As far as I understand this API is part of impress.js (as a whole).
These are common utility functions used both by impress.js core (some of them are directly extracted from impress.js code) and others are shared by many plugins.
I believe it's worth keeping them as part of impress.js to make plugin development easier - not to expect plugins will re-implement same things or rely on other libraries to provide this.
There was a problem hiding this comment.
The code you commented is related to the README implementing a way for users to create snippets inside impress.js namespace through the window.impress.addLibraryFactory function. That's not part of impress.js and is being added with this PR.
If you're referring to the libraries that are already internal to impress.js such as arrayify, byId, $$ and $: what's the point on exposing them for consumers of impress.js? Is it just to make them not to type [...arguments], document.getElementById, document.querySelectorAll and document.querySelector?
We're just obfuscating code. There's no issue requesting that to be exposed.
That's why I believe this PR should make it clear what are the use cases by submitting one plugin at a time and changing impress.js incrementally, just exposing what's necessary for that plugin. This way we'll be able to see how those exposed APIs are being used.
There was a problem hiding this comment.
Leaving the point about usefulness of util functions like $, $$, byId, etc aside.
I think we can agree that if there there is a need to share some functionality and utils between plugins. At least if it is the 'garbage collector' that allows plugins to be correctly destroyed and clean after themselves.
To make it possible for such libraries to register themselves in impress.js some new method is needed to add them. Otherwise impress.js itself will need to be aware what libs to add, which is obviously not really scalable.
I understand the need for addLibraryFactory. If we worry about this being exposed as public API we may 'hide' it and keep private until we think it can be used publicly.
To make impress.js more extensible it needs means (API functions) that add new functionality to it. In such regard addLibraryFactory is a part of impress.js core and valid addition to the API.
There was a problem hiding this comment.
If you're referring to the libraries that are already internal to impress.js such as
arrayify,byId,$$and$: what's the point on exposing them for consumers of impress.js? Is it just to make them not to type[...arguments],document.getElementById,document.querySelectorAllanddocument.querySelector?
Yes, in today browsers we probably don't need utils that and we probably don't need to expose them as public library utils. I understand that they are simply utility functions of current impress.js core moved to the library.
I believe removing them now may affect rest of the plugins @henrikingo has ready to be merged, so again - I'm fine with merging it as it is and reviewing the bigger picture later to see what libs or utils can be removed in favour of native functions.
There was a problem hiding this comment.
I believe removing them now may affect rest of the plugins @henrikingo has ready to be merged, so again - I'm fine with merging it as it is and reviewing the bigger picture later to see what libs or utils can be removed in favour of native functions.
That's cool, it's just that if we leave it exposed we might need to support it as more people start using it.
If we worry about this being exposed as public API we may 'hide' it and keep private until we think it can be used publicly.
That looks like a good approach.
| * the document is in the state it was before calling `impress().init()`. | ||
| * | ||
| * In addition to just adding elements and event listeners to the garbage collector, plugins | ||
| * can also register callback functions to do arbitrary cleanup upon teardown. |
There was a problem hiding this comment.
If we're still goin with the gc pattern, can we add this into the documentation?
There was a problem hiding this comment.
This, and next PRs from @henrikingo are going to introduce changes and new plugins.
Yes, in the end they should be properly documented. But we made a specific dev branch for merging these without releasing. So I'm fine with merging all the work first, trying it out as a whole and then finding the best way to document it - examples for plugins, API docs, etc.
There was a problem hiding this comment.
That's cool, it's just that we might lose track later due to the huge amount of changes. It could be useful to review each PR in isolation so that it's more "manageable" if you get what I mean...
There was a problem hiding this comment.
I know what you mean.
Currently this file has enough documentation in comments to understand what it does and how to use it. So it's enough to give it a review.
Once we have a larger scope (of all the libs and plugins) it may be easier to decide what and how to document.
It's quite likely that because of 'de-centralized' nature of plugins also documentation will not be just one file anymore and will live closer to the code of individual parts.
There was a problem hiding this comment.
Currently this file has enough documentation in comments to understand what it does and how to use it. So it's enough to give it a review.
My point for an incremental PR is not to understand what the GC API does and how to use it. It's to understand how it's being used within a plugin by providing the minimum amount of code and API exposure to satisfy that plugin's requirement, nothing else. That allows us to improve the architecture on top of examples and proper context of how it's being used.
For example, we can start to merge plugins that don't need a lot of exposed APIs to work and expose those APIs as more PRs and plugins are submitted.
Does it makes sense?
There was a problem hiding this comment.
It makes sense in theory. And I would agree with such approach if we would be talking about new things being implemented.
But we are talking about a code that is already implemented, plugins that already rely on libraries, teardown, etc in their current form.
So it would require quite a lot of additional unnecessary work from @henrikingo to carve PRs down to only parts needed for a single PR and plugin.
I suppose this PR feels quite big, because it lays down libraries for all plugins that come later. It will be simpler later to see PRs of single plugins and their usage of libraries.
That's also why I mentioned couple of times that in this case it may make sense to wait with some changes until we have more stuff merged. Because it's not new stuff being implemented and easily changed - it's code already implemented that we want to merge and possibly improve once we have complete picture.
There was a problem hiding this comment.
But we are talking about a code that is already implemented, plugins that already rely on libraries, teardown, etc in their current form.
So it would require quite a lot of additional unnecessary work from @henrikingo to carve PRs down to only parts needed for a single PR and plugin.
Cool, that makes sense for this case, I guess...
| * MIT License | ||
| */ | ||
|
|
||
| ( function( document, window ) { |
There was a problem hiding this comment.
JavaScript has its own Garbage Collector, can't we just keep track of the browser events and create a method to reinitialize impress.js? When the events are removed (.removeEventsListener) they will eventually be garbage collected by the browser.
Each plugin can handle its own means of registering and removing events without having to build a dedicated GC. The only thing they'll do is to attach themselves to impress.js, and that's it.
There was a problem hiding this comment.
I think that 'Garbage collection' may be a bit misleading name for that utility - and that's why it may be confusing.
It's not reimplementing browser JavaScript GC. It's a library that allows impress.js to track all the events/elements that are changed when impress.js is initialized and revert them to their previous state.
can't we just keep track of the browser events and create a method to reinitialize impress.js
This is exactly what this utility does. It collects elements and events modified and created by impress.js (or plugins) and allows to revert them to previous state. Shortly speaking it enables us to have a frequently requested feature of 'uninitializing' impress.js and if needed running init() from fresh state.
I agree that technically it's probably not a 'garbage collection' (becuase it doesn't automatically remove unnecessary or unused object/references). It's rather a registry of elements and events impress.js uses.
It would be good if we could rename it - at least to avoid confusing it with JavaScript GC and giving a false impression that it reimplements something that browser can do automatically - because it doesn't.
There was a problem hiding this comment.
Now that does makes sense. I noticed it was sort of tracking the steps and the registered events. Looks like the term "GC" is a little weird and agree it could be named.
On top of that, are there any chances we can extract some parts of the functionality (at least what doesn't need to have side-effects like pure functions) and unit test it? There's a lot of logic so I assume there's a big chance of future changes by somebody else that is not the author to break impress.js unintentionally?
src/lib/gc.js
Outdated
|
|
||
| // If the above utilities are not enough, plugins can add their own callback function | ||
| // to do arbitrary things. | ||
| var addCallback = function( callback ) { |
There was a problem hiding this comment.
It's a bit hard from the name of it to understand what callback is it, why to add it, when it will be called.
It took me a while (and reading the code) to realise it's simply a callback called on teardown. So I would suggest renaming it to addTeardownCallback which will be more descriptive.
| appendChild: appendChild, | ||
| pushEventListener: pushEventListener, | ||
| addEventListener: addEventListener, | ||
| addCallback: addCallback, |
There was a problem hiding this comment.
@henrikingo I think my biggest complain or worry in this PR will be this GC API.
First of all (already mentioned elsewhere) the name 'garbage collector' is a bit confusing and it would be good to rename it at some point (before we expose it publicly).
For the methods it has in API - it's really hard to tell the difference between pushEventListener and addEventListener without looking at the code of them.
I'm not sure how flexible we need this API.
In theory, the only 'required' thing is addCallback - so that plugins can add a callback and clean up after themselves on teardown. Any other method (registering elements, events, etc) are for convenience.
I don't know how 'convenient' we need this API to be as it already seems confusing.
But maybe what's confusing is names of the functions that are very similar to each other and it's hard to say what they do differently.
I don't know if it's good that we mix low level 'registry' functions like pushElement, pushEventListener addCallback with wrappers over DOM methods appendChild addEventListener.
If we assume that plugins should use convenience methods for DOM operations, maybe we should expose only those.
If we want plugins to do their own DOM operations and only register elements/events, maybe low level functions would be enough?
I'm trying to put myself in a position of potential plugin developer - and looking at this API I don't know what I'm meant to be using.
I kind of feel it's about the balance of being more friendly to less experienced developer (having simpler API) or to more experienced one (having both convenience and low level functions).
I don't have a good answer or suggestion, I just feel that in current form it may be confusing.
@henrikingo What are your thoughts on that?
There was a problem hiding this comment.
I'm trying to put myself in a position of potential plugin developer - and looking at this API I don't know what I'm meant to be using.
From my experience writing 20 plugins for impress.js, and also using same mechanism in impressionist:
- I want methods that wrap around the DOM methods I used until gc was introduced.
- Sometimes I use some other JavaScript libraries that create DOM elements, and I just need to add them to gc so they are removed on teardown. This is what the push* functions do.
- addCallback() is the catch all where a plugin can do anything. But, expecting each plugin to do the addEventListener and appendChild work separately, would be just as annoying and bad style as copy pasting lib.util.triggerEvent to each plugin.
- There's also a need for add* and push* method that would just set and reset an attribute value. This doesn't exist yet.
There was a problem hiding this comment.
So what's the final decision, leave the GC as it is and take a look later once you land the plugins?
There was a problem hiding this comment.
I don't have bigger issues with this PR, but I'd like to hear your comments on 'garbage collector' - it's naming and API.
Would it be possible to rename it now to something less confusing?
Could we limit the list of function it has in API to make it more straightforward?
For sure I'd like these points addressed before we release it to master.
|
After talking with @bartaz, as you can see on the messages, my only concerns are:
|
|
Given what @bartaz said in regards to just move this over into the It looks good to merge then. |
|
@FagnerMartinsBrack comments:
That is a correct observation. As you may remember, in 2015-2016 I wrote some tests for core impress.js, but they were never merged upstream, so I gave up on writing new tests in favor of speeding up feature development. As I was working alone in my own fork, the risk of doing so was a lesser priority. Now the object of this work is to merge 2 years worth of code back upstream, none of which has test coverage beyond what was already merged in my first PR to dev branch. (On the bright side, and kind of because of this, there are several new demo presentations, which indirectly act as testing the new features. None of them test impress().tear() though.) The history that led us down this path is unfortunate, but the situation is what it is. Test coverage will of course have to be added eventually, and now that I'm back as maintainer in impress.js upstream, I do intend to work on that in the future, since I know I can commit that work. But will not do it as part of these merges.
Yes, obviously, but this series of merges is focused on just merging, no new code. @bartaz comments:
The concept and naming of garbage collector comes from Symbian :-) It had a similar mechanism where C++ classes would manually register themselves to a service that would then ensure objects are deleted when application closes. (I think all apps were running in the same process, or something else weird, that caused this kind of unusual need.) I think the name can definitively be changed, just couldn't think of a better one myself. Otoh, a benefit of the current name is that "gc" is a nice and short name for a library that gets used everywhere. In any case, if you can propose a better name (that more clearly describes the library) then I would make such a change at the end of this process, once all plugins are merged to dev branch.
No. As for naming of the library methods, the appendChild and addEventListener are intentionally similar to the same DOM method names, and IMO should stay like that. The push* equivalents are needed for cases where DOM elements are already created by someone else, and you just need to register them for deletion later, so yes, they are needed. The addCallback() method can be renamed to addTeardownCallback() if you prefer, but I would argue that once someone is familiar with what the gc library does, then gc.addCallback() should be quite clear? In fact, as mentioned in the commit message, I feel that there's rather a need for more methods to the API: a pair that would remove/reset an attribute, not the whole element. |
There was a problem hiding this comment.
I think the name can definitively be changed, just couldn't think of a better one myself.
Currently the only one that comes to my mind is 'registry', as this functionality feels to me as a registry of impress-dependent objects. But obviously it doesn't have such a nice shortcut as gc :)
And personally I don't have any experience with similar APIs, so I don't know how such things are called elsewhere.
Could we limit the list of function it has in API to make it more straightforward?
No.
As for naming of the library methods, the appendChild and addEventListener are intentionally similar to the same DOM method names, and IMO should stay like that. The push* equivalents are needed for cases where DOM elements are already created by someone else, and you just need to register them for deletion later, so yes, they are needed.
I disagree on that point, but that's just my opinion.
While I understand the reasoning behind having different methods in this API: low lever push*, convenience DOM equivalents, and 'fallback' addCallback and I understand practical need for all of them to cover different use cases from API 'cleanness' point of view it feels wrong to me to mix them in one 'gc' API.
Especially DOM equivalent methods - these are called as gc.addEventListener of gc.appendChild for example. Are we appending child to garbage collector? Are we adding event listener to gc? No, these methods are named like DOM API methods, but have different arguments (they need additional element argument) and are not really called on gc object, but on element given as argument AND also pushed to GC.
In my opinion it's a bit confusing. But on the other hand I agree that if you have to copy-paste DOM method + gc.push* maybe it's better to just have a convenient way to do both in one go.
So maybe the answer is not in reducing this API, but documenting it and maybe changing the names of the methods to make them more clear?
One idea that comes to my mind is using gc.addEventListenerTo(target, type, handler) and gc.appendChildTo(parent, child)? Maybe this will keep the names similar enough to DOM methods to know what they do, but different enough to understand that arguments are different and they are something else?
It's just a suggestion and I don't mind to leaving it for later - knowing it would have impact on all the plugins now.
The addCallback() method can be renamed to addTeardownCallback() if you prefer, but I would argue that once someone is familiar with what the gc library does, then gc.addCallback() should be quite clear?
Possibly. When I know what it does (that the callback is called on teardown) then I know what it does and I don't need 'teardown' in the name. So again - maybe it's not a problem with API/name but with documentation and/or examples?
One little suggestion - I like how push* methods use the similar naming and distinguish the 'low-level' gc methods. Maybe addCallback should be pushCallback as well?
Thanks for all the answers and more context on the choices here.
If you feel that any of the suggestions can be implemented at this stage in this PR without affecting subsequent PRs to much it would be nice to have them, if not feel free to merge at your convenience.
This gets +1 from me.
I guess it all boils down to us not having the context of how it's being used. I guess we'll be able to provide more meaningful contributions once we see how everything "fits". Maybe with some future Pull Requests on top of all the landed plugins. IMHO this should land on |
I'm glad to have found someone who's taste is even more minimalist than mine!
I admit that documentation for this is maybe a bit sparse. The gc stuff was a lot of work, and it took several commits to move all plugins to using it, and once I got everything working, didn't really spend time documenting it as much as some other parts. Adding better documentation is actually something I CAN do within this PR (just like I've already promised to add impress().tear() documentation).
Yup, this is a great suggestion and easy to agree with. In addition to minimalism, I'm a big fan of consistency in naming. Btw, in case it wasn't obvious already, the push* naming is because gc builds a stack, and when you call teardown, it correspondingly pops items in last-in-first-out order. But I'm afraid this is another thing that is better done later, once all the code is "on the same side" of the big merge.
Thanks both of you. Once I write some better docs, will merge later in the week. |
| console.log( "Impress init" ); | ||
| }); | ||
| impress().init(); | ||
| ``` |
There was a problem hiding this comment.
Probably a copy/paste leftover here, I'll raise a PR to remove it
The next step is to move some of the helper functions into src/lib/*.js files and make them available via impress().lib so that plugins can use them as well. In the code already existing here, navigation plugin uses lib.util.triggerEvent().
The other library is gc, a rather complex "garbage collector" mechanism. It is used to keep track of any changes done to the DOM, mostly as an effect of impress().init(), so that they can be undone with impress().tear(). This was a request in the queue, and I ended up needing it myself too. (The other plugins in my own repo all use this library, so therefore it is merged now.)
@bartaz Based on your comment in the previous PR, I will no longer mark you as a reviewer for PRs, but will continue to ping you so you see these. For this one I will in any case now do something else over the weekend, so it will stay here for review for several days.