Minify the distribution file #98
Comments
|
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. |
|
Sure!! |
|
|
|
As far as I know, There's no ES6 code... |
|
What was the problem you faced? |
|
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? |
|
Ohh okay. I see. |
|
Do you want me to do those changes in this PR or are you going to do those in a separate one? |
|
I first need to check whether that breaks anything else; will do that soon.
…On Fri, Sep 1, 2017, 13:20 Divya Mamgai ***@***.***> 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
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT0xnC5DBfL3kfE27TLAhWNlfx7EGiniks5sd7dXgaJpZM4PEFWE>
.
|
|
Fixed that in #104 |
|
Okay I'll pull the latest and start working on this PR. |
|
Thanks! |
|
Hey, on doing grunt build I'm still getting |
|
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 ***@***.***> 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
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT0xnBgK65RAXXNI1JzPjrG7q8Tc9VGLks5se7-ugaJpZM4PEFWE>
.
|
|
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. |
|
Okay, I'll look into the ES6 supporting solution, it had some issues but I'll keep you posted. |
|
If this is still available id love to have a go at it |
|
Sure, that'd be great!!
…On Thu, Sep 28, 2017, 20:45 makeupsomething ***@***.***> 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
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT0xnHode1C8kYszy7HFTf2oxepNdcilks5sm7gQgaJpZM4PEFWE>
.
|
|
I'm sorry, but I have got some urgent work to take care so yeah you can go ahead and to this. |
|
First timer here. Can I take a stab at this? |
|
Of course, thank you!
…On Fri, Oct 13, 2017 at 9:17 AM, Natalie McMullen ***@***.***> 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
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ2swqhwTRN6wa05UTC5CZMTektEgks5sr2LlgaJpZM4PEFWE>
.
|
|
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. |
|
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 ***@***.***> 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
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ1ZX-5Tp5OFhMwfeql8qtOSOvZ4Mks5sr4KUgaJpZM4PEFWE>
.
|
|
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. |
|
@jywarren @divyamamgai @ccpandhare |
|
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! |
|
Addressed in #124 |
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:
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.jsgenerated above is minified and exported as./dist/image-sequencer.min.js.Steps to Fix
work-neededif you haven't completed the work on the issue)The text was updated successfully, but these errors were encountered: