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

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

Closed
wants to merge 2 commits into from

Conversation

eviltrout
Copy link
Contributor

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
Copy link
Collaborator

ashb commented Sep 4, 2013

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
Copy link
Contributor Author

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
Copy link
Collaborator

ashb commented Sep 4, 2013

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
Copy link
Collaborator

ashb commented Sep 4, 2013

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
Copy link
Contributor Author

  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
Copy link
Contributor Author

@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
Copy link
Collaborator

ashb commented Sep 5, 2013

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

XhmikosR commented Sep 7, 2013

@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).

"grunt-contrib-jshint": "~0.6.4",
"grunt-contrib-uglify": "~0.2.4",
"requirejs": "~2.1.8",
"amdefine": "~0.0.8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you sort these alphabetically? BTW, is your branch based on the latest HEAD? I remember I removed the JSHint dependency.

@eviltrout
Copy link
Contributor Author

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
Copy link
Contributor

XhmikosR commented Sep 9, 2013

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

@eviltrout
Copy link
Contributor Author

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
Copy link
Collaborator

ashb commented Sep 12, 2013

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
Copy link
Collaborator

ashb commented Sep 15, 2013

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 558d7c9

@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 ashb closed this Sep 15, 2013
@ashb
Copy link
Collaborator

ashb commented Sep 15, 2013

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

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

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

Successfully merging this pull request may close these issues.

3 participants