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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial work of Grunt.js-ifiying the repo #10

Closed
wants to merge 5 commits into from
Closed

Initial work of Grunt.js-ifiying the repo #10

wants to merge 5 commits into from

Conversation

pangratz
Copy link

Hey, first of all: very nice stuff! I like the idea of rendering the sequence diagram entirely in the browser 馃憤

I've seen in the README that you plan to use Grunt.js instead of the Makefile, so I gave it a try. I'm not sure if I moved the functionality correctly and I couldn't find out how to test my changes, also I plan to update the README and remove the obsolete Makefile.

I just wanted to open this PR to get some early feedback.

This will replace the Makefile like this:

# install grunt
npm install -g grunt-cli

# install dependencies used in the Grunfile.js
npm install

# execute default task, which creates the sequence-diagram-min.js
grunt

@bramp
Copy link
Owner

bramp commented Mar 24, 2013

Hey Pangratz, looking good thanks!

I've never used grunt, so I don't know if this is the best approach or not. But it certainly removes the "gem install jspp" dependency that bugged me. Feel free to keep making changes, but I'm going to spend a day or so looking at grunt before I merge.

@pangratz
Copy link
Author

I'll let you know when I think the work is done 馃槈

Cheers.

@pangratz
Copy link
Author

Alright, with the latest commit 88c6aff the whole Makefile functionality should be ported to Grunt.js. Though the content of the Makefile-version and Grunt.js-version of the generated sequence-diagram-min.js is not equal, using either version in the test/test.html results in the same output of the sequence diagram. I specified the same parameters for Uglify.js, but somehow the output still differs. Are you OK with that or should I further investigate why there is a difference? I suspect it might have to do something with the comments.

I also thought of further cleaning up the codebase. What do you think of creating a dist directory which will contain the generated files, and hereby also create a un-minified version sequence-diagram.js?

@pangratz
Copy link
Author

I think the main stuff which should be covered by this PR has been done: the Makefile (and the jspp gem dependency) has been replaced by Grunt.js. I've updated the instructions in the README on how to build this library.

The last commit 8ca5210 introduces the output of the un-minified version of sequence-diagram.js. I don't know if this is desired. Maybe I should add this feature via a separate PR so you can decide on this separately... 馃様

@bramp
Copy link
Owner

bramp commented Mar 26, 2013

Hey pangratz, so I've checked out your gruntfile, and I'm now wondering if this is better than plain Makefile.

Pros of Grunt:

  • Handles build dependencies
  • Replaces jspp

Cons of Grunt:

  • Gruntfile is 88 lines, vs 38 lines of Makefile
  • No incremental builds (Makefile only runs the tasks it needs to)

I'm now leaning towards the Makefile, and maybe adapting it to use npm for build deps, and come up with a better replacement for jspp.

@qur2
Copy link
Contributor

qur2 commented Mar 26, 2013

Just my two cents: having a stack full of js tools makes the project easier to get started with. Relying on grunt also helps to easily build a full project around.
Lastly, if I'm not mistaken, grunt brings some explicit and external steps rather than file inclusion hidden in some comment as jspp does.

@pangratz
Copy link
Author

About the lines in the Gruntfile: I am sure it can be further decreased. I have started using Grunt.js in the last weeks and it feels like a very handy tool for building JS stuff, but I'm sure there is potential for improving the file.

About the incremental builds, maybe there is a way to configure the tasks so they are only executed when preconditions are met or not. But I think that creating the lib from a fresh, clean state is not too expensive.

@bramp bramp mentioned this pull request Mar 28, 2013
@darth10
Copy link

darth10 commented Jul 11, 2013

+1

@bramp
Copy link
Owner

bramp commented May 19, 2015

Thanks for this pull request, but I'm going to close it. The Makefile solution works great, it is clean and simple, and now uses npm for all build dependencies.

@bramp bramp closed this May 19, 2015
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.

None yet

4 participants