Introduce plugin framework #622

henrikingo · 2017-09-25T00:06:51Z

Source files are under src/
js/impress.js is now generated, but remains part of the repo (so it just works)
npm run build
build.js uses buildify node module
Break out navigation and resize plugins from core src/impress.js file

* Source files are under src/ * js/impress.js is now generated, but remains part of the repo (so it just works) * npm run build * build.js uses buildify node module * Break out navigation and resize plugins from core src/impress.js file

henrikingo · 2017-09-25T00:09:00Z

No rest for the wicked...

Basically, back in 2015 you were saying you wanted core to remain small and additional features be plugins. I was waiting for you to elaborate, but then realized the end of impress.js already is a plugin. So I've just moved that code out to separate files under src/plugins. Now I'm excited to finally hear whether this is roughly what your vision was?

henrikingo · 2017-09-25T00:13:03Z

js/impress.js

@@ -18,6 +18,7 @@



Note: This is now generated. Makes more sense to comment on the individual files under src/

I'm not exactly sure if keeping generated file in repo is a good thing.
It needs to be kept up to date with rest of the code (you need to remember to build it before commit) and having all the changes doubled makes PRs bit confusing.

That said, I understand the idea behind it (to have it ready to use), so I don't have any other solution now.

Maybe in future we would be able to not have it in repo, but have it built and published to some CDN, so built version would be available on-line, while not being in the repo itself.

(Moving here as the reply works now)

I'm not exactly sure if keeping generated file in repo is a good thing.
It needs to be kept up to date with rest of the code (you need to remember to build it before commit) and having all the changes doubled makes PRs bit confusing.

True. It does of course happen that one forgets to do the final build before commit. We could add an automatic check to prevent that though.

For PRs / git diff I don't personally mind, I get used to skipping that quite fast.

That said, I understand the idea behind it (to have it ready to use), so I don't have any other solution now.
Maybe in future we would be able to not have it in repo, but have it built and published to some CDN, so built version would be available on-line, while not being in the repo itself.

For now, the absolutely biggest issue of course was simply backward compatibility with upstream, and IMO that is important consideration going forward as well. There should be compelling reasons to break with this custom, more than just something being maybe being a bit better. That is to say, I'm not strictly disagreeing with publishing to a CDN or users having to build js/impress.js, but IMO there's also nothing horribly bad with the way it is now and there's value in just continuing with what is already established.

Personally I do like the end user workflow where you can just git clone and point to the js/impress.js file, without, for example, needing to use npm at all. When integrating some of the extras plugins, it always annoyed me how nonstandard that is across projects. For one, you need to download a zip file. For another, you need to npm install. For a third, you need to npm run build dist whatever, and then a lot of new files appear, and you need to figure out which one you actually want. Ease of use of impress.js is really good right now, so lets try to keep it.

henrikingo · 2017-09-25T00:15:21Z

src/impress.js

+
+/*jshint bitwise:true, curly:true, eqeqeq:true, forin:true, latedef:true, newcap:true,
+         noarg:true, noempty:true, undef:true, strict:true, browser:true */
+/*global window*/


I realize now linting actually passes without declaring 'window'. Or 'document' for that matter.

I don't remember now exactly, but I suppose browser: true setting is enabling all browser globals, so global: window should not be needed here.

henrikingo · 2017-09-25T00:17:07Z

src/plugins/README.md

@@ -0,0 +1,407 @@
+Impress.js Plugins documentation


This is the current plugins/README.md from my fork. It refers to things not yet possible in this PR, but I thought I'd rather not start editing it into smaller pieces. This also gives you the full picture, even if some plugin types will be introduced in a later commit.

OK. We are merging to dev branch anyway, so there is no risk of confusing external people at this point.

It looks like a nice and detailed documentation, thanks.
I don't have time to read it all now, but will try to have a closer look later. (but we don't need to address it in this PR).

bartaz

Thanks.

Yes, the initial idea was something like this - as you've noticed navigation and resize were already separated in the code, just not in separate files to avoid build steps.

So for sure I think it's a good start and in a good direction. I'll read your plugins README later in more detail and maybe comment on some bits there, but generally it looks good.

The broader vision I had was for the core of impress.js API to be limited almost just to animating between different 'positions' in 3d space - not even steps.
So for example impress code would have a function impress().goTo(x, y, z, scale, rotate) (not sure exactly how many params are needed to define all the props of 'camera' position and 'steps' functionality would be a plugin that would allow to define steps of the presentation (and move between them using this code 'goTo' function).
But that's just an idea, not sure if it's even practical or worth aiming for. Just wanted to give you some more context on my thinking.

Knowing that you already have more functionality implemented in plugins let's first get that and then think where to go next.

I'm not approving this PR yet (to keep discussion open in comments for a bit), but I don't see anything blocking at the moment.

bartaz · 2017-09-25T07:20:34Z

build.js

+           'src/plugins/resize/resize.js'])
+  .save('js/impress.js');
+/*
+ * Disabled until uglify supports ES6: https://github.com/mishoo/UglifyJS2/issues/448


There is now uglify-es module for ES6 support, so we would need to switch to that to support it.

Not that it needs to be done in this PR, but just a note for future.

Cool. I'll keep that in mind as a future improvement. I really like to provide the minified experience, but felt that not adopting language improvements was eventually holding back progress too much.

bartaz · 2017-09-25T07:23:26Z

js/impress.js

@@ -18,6 +18,7 @@



I'm not exactly sure if keeping generated file in repo is a good thing.
It needs to be kept up to date with rest of the code (you need to remember to build it before commit) and having all the changes doubled makes PRs bit confusing.

That said, I understand the idea behind it (to have it ready to use), so I don't have any other solution now.

Maybe in future we would be able to not have it in repo, but have it built and published to some CDN, so built version would be available on-line, while not being in the repo itself.

bartaz · 2017-09-25T07:32:00Z

src/impress.js

+
+/*jshint bitwise:true, curly:true, eqeqeq:true, forin:true, latedef:true, newcap:true,
+         noarg:true, noempty:true, undef:true, strict:true, browser:true */
+/*global window*/


I don't remember now exactly, but I suppose browser: true setting is enabling all browser globals, so global: window should not be needed here.

bartaz · 2017-09-25T07:39:45Z

src/plugins/README.md

@@ -0,0 +1,407 @@
+Impress.js Plugins documentation


OK. We are merging to dev branch anyway, so there is no risk of confusing external people at this point.

It looks like a nice and detailed documentation, thanks.
I don't have time to read it all now, but will try to have a closer look later. (but we don't need to address it in this PR).

FagnerMartinsBrack

This is a good separation IMHO.

Now that we've got the public API hanging on for some time we might be able to do some changes knowing what we can break and what we can't. The source doesn't need to be that rigid.

The only concern are people using the impress.js file from master, but in that case there's not much we can do.

1 years ago I wrote a RoadMap to serve as guidance for whoever might have looked after contributing something. I believe if we wait for proper testing we'll never move forward. Is that something that's still relevant?

LGTM after @bartaz comments are addressed.

^{I'm sorry I couldn't provide more useful contributions other than public docs and something here and there. @henrikingo have been doing a great job on the fork.}

bartaz · 2017-09-25T13:00:00Z

@FagnerMartinsBrack

Thanks for joining the discussion.

The project was obviously stuck and having a great plan of actions is not worth much when no one is doing them. So you are right, that waiting for a perfect test suite is just not moving project anywhere.

@henrikingo in his fork has improved impress.js API and developed couple of plugins (including substeps, etc - that were often requested). So this PR (and others that will follow) are simply attempt to move his work back to upstream impress.js and let him develop it more here, where his code can actually reach most of impress.js users.

For now, not to break anything in master, it's all merged to a new dev branch that I suppose we will merge to master once all the code will be there and we will clean up any conflicts or broken things.

And no need to be sorry - we all do it in free time so each contribution is appreciated.
For now I guess let's not 'overplan' or 'overmanage' on things we cannot deliver, and let's focus on moving the code that is already done back to impress.js.

henrikingo · 2017-10-02T08:45:44Z

I'm not approving this PR yet (to keep discussion open in comments for a bit), but I don't see anything blocking at the moment.

Is this ok to merge soon, or are you still going to comment on something?

(Beyond the unnecessary declarations of window and document, which I can remove.)

bartaz · 2017-10-02T08:55:25Z

@henrikingo I was just looking at this PR to ask the same question :)

If you are fine with the branch as it is right now, go ahead and merge it (I assume as a member of the organisation you can do it yourself).
The only unaddressed comment from my side is about possibly unnecessary global: window. Which you can address now or later, as you wish. It's not that important.

In future branches not to make you feel blocked on not getting answers/comments - when PR got approval already, just merge it once you feel you addressed all the comments that you feel needed addressing. Are you ok with such process?

henrikingo · 2017-10-02T21:08:10Z

In future branches not to make you feel blocked on not getting answers/comments - when PR got approval already, just merge it once you feel you addressed all the comments that you feel needed addressing. Are you ok with such process?

Yes and no :-) As we discussed at the start, you are the creator of this project, and I'm really glad you've stepped up to make these two reviews. But once you feel you've done your part, and want to step aside again, there's nobody else that has written as much impress.js code as you, and me second. So I'm happy to receive comments from anyone in the community, but I don't intend to wait for approvals from anybody else than yourself.

As for next steps, I think these two PRs were the foundational ones, and it's great that you were able to confirm you approve of the direction they set. The next two may be somewhat interesting as well, and I will give you some time to react to them, just in case you want to. The next two upcoming PRs will be:

shared functions in src/lib. (Not plugins, rather just utility code shared across everything and called synchronously, not via events.)
pre-init and pre-stepleave plugins. These types are under src/plugins, but executed synchronously before their respective events happen.

After those two it's just lots of plugins and some new demo presentations to showcase / document / test new functionality. Reviewing all of them might be boring to you, but I'll shuffle them through as PRs nevertheless. It will help everyone else too to follow what goes on.

We're still merging to the dev branch anyway. So once everything is merged, I intend to post an issue with a proposal for how and when to merge that into master, that people can comment on.

FagnerMartinsBrack · 2017-10-09T08:21:39Z

@henrikingo I'm not sure why, but after this commit on master, SauceLabs started failing on CI.

Do you have access to it?

See the screenshot below:

FagnerMartinsBrack · 2017-10-09T08:22:19Z

As you can see in the top, I tried to run the last passing build to see if there was some racing condition, but it looks like it's failing consistently.

https://circleci.com/gh/impress/impress.js/80

henrikingo · 2017-10-09T10:07:40Z

I have never run the karma.conf-sauce.js in my fork. I was surprised to see it pass when I submitted the first PR, so didn't remove from circle.yml. We of course need to look into it later, but the short answer is I have no idea.

I can't see the output of the failing stage above. It just spins forever.

FagnerMartinsBrack · 2017-10-09T11:17:40Z

@henrikingo Can you make sure you can see the stacktrace, have a Circle CI account and has write access to the impress.js org?

henrikingo · 2017-10-10T12:31:15Z

Ah, I just wasn't logged in.

02 10 2017 20:56:12.011:WARN [Chrome 53.0.2785 (Windows 10 0.0.0)]: Disconnected (1 times), because no message in 10000 ms.
Chrome 53.0.2785 (Windows 10 0.0.0): Executed 0 of 2 DISCONNECTED (10.156 secs / 0 secs)
02 10 2017 20:56:12.013:ERROR [reporter.sauce]: ✖ Test Disconnected

I vaguely remember that this happened to me in karma.conf.js as well, and I increased a timeout to get it to work: https://github.com/impress/impress.js/blob/dev/karma.conf.js#L67

FagnerMartinsBrack · 2017-10-11T07:57:45Z

That doesn't seem to be working now. Can we trace down the root cause of the issue? The build should always be passing, otherwise, it misses the point.

henrikingo · 2017-10-11T08:58:41Z

I will of course make sure tests pass. So far all PRs have been green. It seems CircleCI doesn't send me any notification of failing builds even if I'm now an impress.js maintainer. Could you fix that?

FagnerMartinsBrack · 2017-10-11T09:37:42Z

@henrikingo Not sure if it depends on the project settings, you can setup the notifications in the "User Settings": https://circleci.com/account/notifications

FagnerMartinsBrack · 2017-10-11T09:42:05Z

So far all PRs have been green.

They didn't because Sauce Labs wasn't run in your PRs. See https://circleci.com/gh/henrikingo/impress.js/86?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link and:

npm run test:sauce

> impress.js@0.6.0 test:sauce /home/ubuntu/impress.js
> karma start karma.conf-sauce.js

Sauce environments not set --- Skipping

I'm not sure why they weren't. When I created a PR as the author, it did run and was failing: #627

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce plugin framework #622

Introduce plugin framework #622

henrikingo commented Sep 25, 2017

henrikingo commented Sep 25, 2017

henrikingo Sep 25, 2017

bartaz Sep 25, 2017

henrikingo Sep 26, 2017

henrikingo Sep 25, 2017

bartaz Sep 25, 2017

henrikingo Sep 25, 2017

bartaz Sep 25, 2017

bartaz left a comment

bartaz Sep 25, 2017

henrikingo Sep 26, 2017

bartaz Sep 25, 2017

bartaz Sep 25, 2017

bartaz Sep 25, 2017

FagnerMartinsBrack left a comment •

edited

Loading

bartaz commented Sep 25, 2017

henrikingo commented Oct 2, 2017

bartaz commented Oct 2, 2017

henrikingo commented Oct 2, 2017

FagnerMartinsBrack commented Oct 9, 2017

FagnerMartinsBrack commented Oct 9, 2017 •

edited

Loading

henrikingo commented Oct 9, 2017 •

edited

Loading

FagnerMartinsBrack commented Oct 9, 2017

henrikingo commented Oct 10, 2017

FagnerMartinsBrack commented Oct 11, 2017

henrikingo commented Oct 11, 2017

FagnerMartinsBrack commented Oct 11, 2017 •

edited

Loading

FagnerMartinsBrack commented Oct 11, 2017

FagnerMartinsBrack commented Oct 11, 2017

jdalton commented Oct 11, 2017 •

edited

Loading

FagnerMartinsBrack commented Oct 17, 2017 •

edited

Loading

Introduce plugin framework #622

Introduce plugin framework #622

Conversation

henrikingo commented Sep 25, 2017

henrikingo commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FagnerMartinsBrack left a comment • edited Loading

Choose a reason for hiding this comment

bartaz commented Sep 25, 2017

henrikingo commented Oct 2, 2017

bartaz commented Oct 2, 2017

henrikingo commented Oct 2, 2017

FagnerMartinsBrack commented Oct 9, 2017

FagnerMartinsBrack commented Oct 9, 2017 • edited Loading

henrikingo commented Oct 9, 2017 • edited Loading

FagnerMartinsBrack commented Oct 9, 2017

henrikingo commented Oct 10, 2017

FagnerMartinsBrack commented Oct 11, 2017

henrikingo commented Oct 11, 2017

FagnerMartinsBrack commented Oct 11, 2017 • edited Loading

FagnerMartinsBrack commented Oct 11, 2017

FagnerMartinsBrack commented Oct 11, 2017

jdalton commented Oct 11, 2017 • edited Loading

FagnerMartinsBrack commented Oct 17, 2017 • edited Loading

FagnerMartinsBrack left a comment •

edited

Loading

FagnerMartinsBrack commented Oct 9, 2017 •

edited

Loading

henrikingo commented Oct 9, 2017 •

edited

Loading

FagnerMartinsBrack commented Oct 11, 2017 •

edited

Loading

jdalton commented Oct 11, 2017 •

edited

Loading

FagnerMartinsBrack commented Oct 17, 2017 •

edited

Loading