evilstreak / markdown-js

A Markdown parser for javascript
7.69k stars 863 forks source link

Split up markdown-js into multiple files, build system assembles from them for web use. #113

Closed eviltrout closed 10 years ago

eviltrout commented 10 years ago

This is quite a big change, so here's a big explanation:

The project has been broken up from one large file into several smaller files:

/core.js defines Markdown /markdown_helpers.js includes the often used helpers /parser.js includes the code relevant to parsing a string into JsonML /render_tree.js the code for producing a string from the JsonML. /markdown.js The main file that includes all the others. /dialects/gruber.js The code relevant to the Gruber dialect of markdown /dialects/maruku.js The code relevant to the Maruku dialect

Note: No functionality should have changed besides that necessary to split it up and import the helper functions where necessary. A lot of lines changed, but most of those lines are exactly the same as before. All tests pass.

Usage

If you are using node, you can simply include the library as before and everything is the same.

If you'd like to use markdown-js in a browser, there is an additional build step that is required via grunt:

$ grunt all

It will run all the tests and produces dist/markdown.js and dist/markdown.min.js, which you are free to use in a browser to your heart's content. It strips out all the AMD stuff and concatenates it altogether nicely.

One more thing

You can tell grunt to build a custom markdown-js. For example if you don't want Maruku:

$ grunt custom:-dialects/maruku

If you do this, your build file will not include any of the code from dialects/maruku. Pretty sweet!

ashb commented 10 years ago

So haven't had a chance to look over this in detail yet, but a quick glance and my question is: amdefine - what does this give us? Would it work in a browser without an extra dep? Do we get anything for using this over just concatenating the files together in the build process?

eviltrout commented 10 years ago

The goal was to have a client side build that is concatenated without any dependencies. I basically copied the way jQuery does this. Essentially during the build phase it strips out all the define statements (including amdefine). There are no additional client side dependencies. You can just use the markdown.js that it spits out as before.

Server side (node) is a little different, as it does not use the built version. It actually loads the dependencies as needed and ordered in the define statements. So the amddefine helper is required in this case. It is annoying to add another dependency, but it's a very short one and works quite well.

The amd define line at the beginning of each file is a bit ugly I agree, but it allows us to use the same files in a node app and to tell the build process how to order and concatenate.

ashb commented 10 years ago

Do you have a good intro to amd? I've not really followed it but I have never been quite sure on what it gives over just the built in require.

Oh also you've got a .DS_Store added in git

ashb commented 10 years ago

Oh and two minor things:

1) Could we change the dirs used? build/ to me sounds like the output dir, not an input dir. Could we change this dir to something else? maybe inc/ ?

2) Will grunt build build with all dialects by default?

Definitely like it on general terms - we (evilstreak and I) were actually thinking about this a few weeks ago - some way of splitting up the giant markdown.js file was needed.

eviltrout commented 10 years ago

1) I've renamed /build to be inc/, no problem there.

2) By default grunt build or just grunt builds all dialects.

I'm going to look into whether we can do the AMD magic with pure require statements now. I honestly chose AMD because jQuery did that. It might be possible without the amdefine dependency.

eviltrout commented 10 years ago

@ashb I looked into the AMD versus require thing. What a complicated ecosystem we live in with JS -- There is AMD, CommonJS (node style require) and browser globals!

The main advantage of using AMD for the project is that we have a build script (ported from jQuery, also MIT) which uses requirejs to concatenate all the files together intelligently, respecting their dependencies on each other.

The jQuery script also allows us to use that minus syntax for not including some dialects and it knows how to remove all the define statements when building the final browser JS file.

The disadvantage is we need the amdefine shim to have AMD work in a node environment as well as the line of code that detects AMD before we use it in each file.

amdefine is a very small dependency (300 LOC including comments) and in node it ends up doing little more than translating the AMD syntax into requires behind the scenes so I imagine the overhead is insignificant.

If we wanted to remove it, we would have to rebuild the build script to handle dependencies with require instead, to strip out the require lines and to support excluding certain files. It's doable but I imagine a lot more work. Personally I am a fan of the amdefine but I would appreciate your input.

ashb commented 10 years ago

So it sounds like having amdefine gives us a useful benefit to require. I wonder if it would be possible to ship a 'compiled' version when doing npm publish... given what I remember of Isaac's views on CoffeeScript modules (i.e. you should be writing in CS and shipping JS) I think it might be possible.

Thoughts?

eviltrout commented 10 years ago

@ashb It seems it is possible to do that. I could make amdefine a dev dependency then and ship a compiled version that is concatenated like the front end file. I'll take a stab at that!

eviltrout commented 10 years ago

Okay, here's a new version! amdefine is no longer a dependency and has been made into a devDependency.

When you build it will run two different builds, one to create the web version and one to create the node version (which ends up in lib and is in .gitignore).

There is a prepublish script to make sure it's compiled before the module is published to npm.

I've tested in a browser and a simple node app and it all seems to work well.

XhmikosR commented 10 years ago

@eviltrout: Can you enable JSHint's indent:2 option? I think your changes will make the code use consistent indentation thus this is a good opportunity to enable it and fix any relevant issues (shouldn't be a lot with your changes).

eviltrout commented 10 years ago

Here's a new commit with the latest changes (rebased against master, alphabetized dependencies, split minimum node version into a different commit.)

@XhmikosR I tried JSHint's indent:2 option and it broke in many places. My code is clearly not enough to address that yet, perhaps that should come in another PR.

XhmikosR commented 10 years ago

No worries. Maybe someone else will take care of it later.

eviltrout commented 10 years ago

Anything else I can do to get this PR accepted? I'd like to add more stuff to the project but this is really a big pre-requisite for more development.

ashb commented 10 years ago

I'm happy with the general approach. @evilstreak or I will hopefully get round looking at merging/making any final tweaks this weekend.

If you are happy to rebase your work then go ahead and start with this branch as your base.

ashb commented 10 years ago

Fixed a few minor things (tabs instead of spaces in the inc/ files, small readme tweak, and fixed and enabled the JSHint indent check and fixed what it was complaining about) - the merge commit was 558d7c960b63529942f413b1b27adad5655cf385

@XhmikosR thanks for your comments on this branch. Can't say how good it feels to have other people care enough about some code you wrote to weigh in on branches they aren't involved in :D

@eviltrout Thanks! Much nicer now.

ashb commented 10 years ago

Turns out GitHun brought back the download feature as another name:

https://github.com/evilstreak/markdown-js/releases/tag/v0.6.0-beta1