Image-sequencer: Minify the distribution file

[ccpandhare]

First Timers Only

Hi, this is a first-timers-only issue. This means this has been worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If you have contributed before, consider leaving this one for someone new, and looking through our general help wanted issues. Thanks!

The Problem

Image Sequencer has a lot of source files and hence the distribution ends up being pretty large, we can do our bit by minifying the distribution file.

All the source files are located in ./src/

The Solution

Grunt has different modules for different tasks. One such module is grunt-contrib-uglify. This package will uglify(or minify) your JS files so that they take less space (by removing whitespaces, line breaks and comments).

What you'll have to do is install this module as a developer dependancy by doing this:

image-sequencer $ npm install --save-dev grunt-contrib-uglify

Now you can use this in Image Sequencer. What grunt does is defined in the Gruntfile ( ./Gruntfile.js )

There is a task "build" defined in the Gruntfile. In the build task, what is already being done is that the src files are browserified into one single distribution file.

In the same task, add the grunt-contrib-minify such that the ./dist/image-sequencer.js generated above is minified and exported as ./dist/image-sequencer.min.js.

Steps to Fix

• [ ] Claim this issue with a comment here, and ask for any issues/clarifications.
• [ ] Clone this repository locally.
• [ ] Try to fix the issue as described above, but even before you have completed doing so, you can:
• [ ] Commit your work and initiate a Pull Request (Mark it as work-needed if you haven't completed the work on the issue)

[divyamamgai]

Can I take up this issue? I want to get started with Open Source contribution and this seems a good place to start since I already have experience with this.

[ccpandhare]

Sure!!
:-)

[divyamamgai]

Can you guide me as to how do I initiate the build?
Looks like we are using ES6 and uglify doesn't provide support for that. I have found a solution here - How to integrate uglify-es in grunt?, tell me if this is fine. Also I don't see a linting file should we add that too?

[ccpandhare]

As far as I know, There's no ES6 code...
The dependencies might be using ES6.

[ccpandhare]

What was the problem you faced?

[divyamamgai]

const keyword is used which is in the spec for ES6 but not the previous ones, so uglify is throwing unknown keyword exception. Maybe we can just change them to var?

[ccpandhare]

Ohh okay. I see.
Yes I think that can be done. Will have a look. Thanks!

[divyamamgai]

Do you want me to do those changes in this PR or are you going to do those in a separate one?

[ccpandhare]

I first need to check whether that breaks anything else; will do that soon.

On Fri, Sep 1, 2017, 13:20 Divya Mamgai [e-mail] wrote:

Do you want me to do those changes in this PR or are you going to do those
in a separate one?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/98#issuecomment-326514594,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AT0xnC5DBfL3kfE27TLAhWNlfx7EGiniks5sd7dXgaJpZM4PEFWE
.

[ccpandhare]

Fixed that in #104
Thanks for you patience and sorry for the delay!

[divyamamgai]

Okay I'll pull the latest and start working on this PR.

[ccpandhare]

Thanks!

[divyamamgai]

Hey, on doing grunt build I'm still getting const variables in the dist js, like I8 and I64. Can you help? I have forked the latest master.

[ccpandhare]

Ohh is it? Will have a look. Thanks!

I think it is pretty safe to replace const with var, because we haven't
used them for a particular reason.

Will check.

On Mon, Sep 4, 2017, 14:45 Divya Mamgai [e-mail] wrote:

Hey, on doing grunt build I'm still getting const variables in the dist
js. Can you help? I have forked the latest master.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/98#issuecomment-326910706,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AT0xnBgK65RAXXNI1JzPjrG7q8Tc9VGLks5se7-ugaJpZM4PEFWE
.

[ccpandhare]

I checked. Some more "const"s were there; I removed the const identifiers from our code, but one of our dependencies has it. So we will have to go ahead with a solution which supports ES6.

[divyamamgai]

Okay, I'll look into the ES6 supporting solution, it had some issues but I'll keep you posted.

[makeupsomething]

If this is still available id love to have a go at it

[ccpandhare]

Sure, that'd be great!!

On Thu, Sep 28, 2017, 20:45 makeupsomething [e-mail]
wrote:

If this is still available id love to have a go at it

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/98#issuecomment-332869133,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AT0xnHode1C8kYszy7HFTf2oxepNdcilks5sm7gQgaJpZM4PEFWE
.

[divyamamgai]

I'm sorry, but I have got some urgent work to take care so yeah you can go ahead and to this.

[nataliemcmullen]

First timer here. Can I take a stab at this?

[jywarren]

Of course, thank you!

On Fri, Oct 13, 2017 at 9:17 AM, Natalie McMullen [e-mail]
wrote:

First timer here. Can I take a stab at this?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/98#issuecomment-336449676,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ2swqhwTRN6wa05UTC5CZMTektEgks5sr2LlgaJpZM4PEFWE
.

[divyamamgai]

Is this still open? I see that in the master uglify task has been added and is working using the harmony branch of grunt-contrib-uglify.

[jywarren]

ah, perhaps you're right; @ccpandhare, if this is complete could you close
this issue?

Divya - do any other issues look interesting to you? Thanks!

On Fri, Oct 13, 2017 at 11:32 AM, Divya Mamgai [e-mail]
wrote:

Is this still open? I see that in the master uglify task has been added
and is working using the harmony branch of grunt-contrib-uglify.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/98#issuecomment-336487034,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ1ZX-5Tp5OFhMwfeql8qtOSOvZ4Mks5sr4KUgaJpZM4PEFWE
.

[divyamamgai]

Sorry, but currently I'm occupied with something. I'll finish that up in two weeks. After that I will take up an issue to resolve.

[makeupsomething]

@jywarren @divyamamgai @ccpandhare
Sorry, i made a pull request for this issues a while back and didn't realise it had been merged already.

[jywarren]

Ah, well, it looks like this is indeed resolved so I'll close it. Sorry for the confusion folks, and we've been adding new issues we'd love help with! Thank you all very much!

[jywarren]

Addressed in https://github.com/publiclab/image-sequencer/pull/124