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

[WIP] Plugin Experiment #336

Closed
wants to merge 22 commits into from

Conversation

Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan commented Jun 15, 2017

I thought I'd play around with the concept of plugins (#163) in a private fork to see how something like this could look.

I doubt anything from this PR will end up in master, but it'll give us a more informed idea for what works, what doesn't work, how much effort could be involved in implementing it properly, etc.

Note: I've started this branch on top of #335 because I needed some of the refactorings done there. I'm not sure if this is a no-no in terms of git branching and all that, so let me know if I need to fix any history.

@Michael-F-Bryan
Copy link
Contributor Author

To make it easier to integrate plugins into configuration and the rendering process, it may be a good idea to break the MDBook up into a couple smaller types. Off the top of my head, I'd probably break it up like this:

  • Book: a plain old container which keeps track of its chapters and configuration
    • Has no real behaviour or responsibilities except from a bunch of getters and setters
  • DirectoryManager: the object in charge of the src/ directory, it will
    • create a new book and source tree from a template
    • Parse a SUMMARY.md and source files into the Book struct
  • Orchestrator: the object in charge of tying everything together, it
    • Defers to the DirectoryManager to load the Book from disk
    • sets up the rendering pipeline so the Renderer can transform source text into the final product, while making sure each Plugin is called so they can do their pre/post-processing

I feel like MDBook has grown over time and gained functionality, and now it's trying to do too much, making it hard to extend or add different things like an alternate renderer backend or plugins. It's also fairly closely tied to the HTML renderer.

For example a MDBook probably doesn't need to know about CSS or JS explicitly. Instead, when the Orchestrator instantiates a new Renderer (it might have looked in the config and seen a renderer = "html" entry), it'll pass in a reference to the config. Then the Renderer can figure out how to set itself up, as well as look for a table with its name on it for Renderer-specific options. So the HTMLRenderer would look for a table in the config called "html", the PDFRenderer would look for a "pdf" table, etc.

@azerupi, what are your thoughts? After next Wednesday I've got a month or two off uni, so I'll have plenty of time to work on this.

@Michael-F-Bryan
Copy link
Contributor Author

I believe solving this (#163) in an extensible way will probably end up also having to deal with #90. So we'd be potentially killing 2 birds with 1 stone.

@azerupi
Copy link
Contributor

azerupi commented Jun 16, 2017

This is very exciting!
I haven't had the time to look at the code yet, so I can't give a lot of feedback.

I feel like MDBook has grown over time and gained functionality, and now it's trying to do too much, making it hard to extend or add different things like an alternate renderer backend or plugins. It's also fairly closely tied to the HTML renderer.

That is a correct assessment 😉
When I started this project I didn't really think it would come this far, so I didn't put a lot of thought into it. And then people got interested and asked for new features, so I tried to deliver. Unfortunately I often went the quick route instead of the durable one. Anyways, a big refactor is absolutely necessary at this point.

I believe solving this (#163) in an extensible way will probably end up also having to deal with #90.

I think so too, plugins would probably need most of the other changes to work.

For example a MDBook probably doesn't need to know about CSS or JS explicitly.
It's also fairly closely tied to the HTML renderer.

Absolutely, I didn't start with the idea of having multiple renderers. We need to progressively decouple the html renderer so that it is completely isolated. But I think it not going to be a quick fix. 😋 And renderers should know what resources they need, mdBook should only tie everything together, like you say.

I am very very interested in this! So if you want to take on the work, I can only encourage you! I'm open for discussions and questions. Feel free to make new issues when you want to discuss a certain topic.

@Michael-F-Bryan
Copy link
Contributor Author

It's an achievement in itself that mdbook has been so popular and evolved to the point where you need to rethink its design! The vast majority of my personal projects lose momentum after a couple weeks and end up being forgotten and half finished. As it is, I've used mdbook several times now and found it to be excellent so it's nice to give something back.

Because this will largely be a structural change with me mainly rearranging existing code, I thought I might approach the task by creating an empty sub-crate (arbitrarily named mdbook-core) and then copy/pasting things across one at a time. It'll probably be a bit more time-consuming, but that way I've got the flexibility to structure things any way I want and it'll be easier to discover which components are unnecessarily coupled in the existing system. Do you think this is the right approach to take?

So far I've copied across the summary parsing and set up a lot of the base types (Runner, Book, Loader, etc). I've also added a couple tests here and there to help me figure out what various things are doing, I figure they'll be useful in the future in preventing regression.

@azerupi
Copy link
Contributor

azerupi commented Jun 17, 2017

I thought I might approach the task by creating an empty sub-crate (arbitrarily named mdbook-core)

Seems like a sensible approach :)
I propose we try to merge and discuss changes on a regular basis because I'm terrible at reviewing large diffs.. 😉

I've also added a couple tests here and there to help me figure out what various things are doing, I figure they'll be useful in the future in preventing regression.

That's awesome, a lot of things are untested right now and it sometimes comes back to bite us. So adding more tests is already a big improvement in itself! 😊

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jun 17, 2017

I propose we try to merge and discuss changes on a regular basis because I'm terrible at reviewing large diffs..

I was thinking about this as well. I don't really want to do the entire refactoring in a single pull request because a lot of stuff will be moved around and it'll probably end up with a 6000-odd line diff. Plus if I'm incrementally moving things across then other people can help out, I can use the issue tracker, and it'll help to break the task down into manageable chunks.

What are your thoughts on merging an initial implementation that's pretty much just a skeleton with a bunch of stubbed out types and unimplemented!() functions?

Also, after we find some nice way of laying things out, what are your thoughts on asking for help in the Call for Participation thread in the forums? We could easily make a bunch of easy issues along the lines of "copy X to mdbook-core and make sure everything compiles", which would be nice as an easy first PR.

@Michael-F-Bryan
Copy link
Contributor Author

@azerupi, when you have a spare couple minutes, would you be able to have a look at what I've done so far?

The easiest way will probably be to clone my fork (git clone https://github.com/Michael-F-Bryan/mdBook), checkout the plugins branch, then go into the mdbook-core directory and build the docs. That's what I've been doing when I want to get a high-level view of what I've done so far (plus it forces me to document what things do).

@azerupi
Copy link
Contributor

azerupi commented Jun 17, 2017

I will check it out in the next couple of days. I still have a big project that is due Wednesday for uni, but I should be able to take some time to check out your changes. Stay tuned!

What are your thoughts on merging an initial implementation that's pretty much just a skeleton with a bunch of stubbed out types and unimplemented!() functions?

Sure I'm ok with that. So if I understand correctly, we would keep the current mdbook crate and continue parallel development in the mdbook-core crate?

If we do this, I think mdbook should relatively rapidly remove some of its components and use the core as soon as possible in order to avoid diverging too much. If people continue to fix issues and contribute features we don't want to have to back-port everything constantly. What do you think?

what are your thoughts on asking for help in the Call for Participation thread in the forums?

Definitely, I have made changes to the repo recently to make it more contributor friendly. I am always happy to mentor and help others contribute. :)

@Michael-F-Bryan
Copy link
Contributor Author

If we do this, I think mdbook should relatively rapidly remove some of its components and use the core as soon as possible in order to avoid diverging too much.

I was worried about this too. After thinking a bit more about this I'm not sure my mdbook-core approach is the right one. It'll mean we have two versions of mdbook being worked on concurrently and is pretty much the equivalent of a rewrite, just with a lot more copy/paste (obligatory Joel on Software article on why this may be a bad idea). Instead I think I'll try to do lots of incremental restructurings of the existing codebase to make it less coupled and more testable. With the end goal to have it structured similar to what I've done in this PR.

I'll still keep my plugins branch around because that contains an "ideal" version of how I think mdbook could be structured in the future and it'll be a nice reference for where I want to end up. My last exam is on wednesday, so expect to see a lot of smaller pull requests coming in over the next month or so.

I doubt anything from this PR will end up in master, but it'll give us a more informed idea for what works, what doesn't work, how much effort could be involved in implementing it properly, etc.

Haha, turns out my comment at the very start of this PR was pretty accurate! It did give me a much better understanding of the task on hand and how mdbook works under the covers though, so this definitely hasn't been a waste of time.

@azerupi
Copy link
Contributor

azerupi commented Jun 18, 2017

Perfect, let's do that :)
You can leave the PR open so that other people can see it and join in.
I'm going to make a new milestone for plug-ins, don't hesitate to create a bunch of issues / todos on the issue tracker as you see fit :)

@azerupi azerupi added A-Internal-representation Area: Internal Representation M-Discussion Meta: Discussion S-Experiment Status: Experiment C-refactor Category: Code refactoring labels Jun 18, 2017
@azerupi azerupi added the S-Work-in-progress Status: Work-in-progress label Jun 26, 2017
@Michael-F-Bryan Michael-F-Bryan deleted the plugins branch August 21, 2017 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Internal-representation Area: Internal Representation C-refactor Category: Code refactoring M-Discussion Meta: Discussion S-Experiment Status: Experiment S-Work-in-progress Status: Work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants