Image-sequencer: Managing dist files

[VibhorCodecianGupta]

Please describe the problem (or idea)

Committing the dist files with every issue results in merge conflicts every time, with a constant need for rebasing. This is particularly uncalled for in case of image sequencer, given that almost every issue is concerned with different modules and the changed files shouldn't conflict at all, which is the whole point of a modular structure, so changes somewhere do not require churning the entire codebase. It is just the dist files that cause the inconvenience.

Possible workaround

Since Image-sequencer uses semantic versioning, It could be made a standard contributor practice to NOT commit the dist files while resolving issues. The project maintainers can once in a while build the dist files, commit them, and make the version bump. (I think it is 2.3.3 as of now.)
OR, critical bug fixes could be the issues requiring committing the dist files and feature additions can be committed without them. Again, once the maintainers feel that enough changes have been made, they can build and commit the dist files and make a version bump.

@jywarren @tech4GT thoughts?

---

Thank you!

Your help makes Public Lab better! We deeply appreciate your helping refine and improve this site.

To learn how to write really great issues, which increases the chances they'll be resolved, see:

https://publiclab.org/wiki/developers#Contributing+for+non-coders

[gitmate[bot]]

GitMate.io thinks possibly related issues are https://github.com/publiclab/image-sequencer/issues/98 (Minify the distribution file), https://github.com/publiclab/image-sequencer/issues/141 (move module tests into separate test files), and https://github.com/publiclab/image-sequencer/issues/26 (Break out longer functions into sub source files and require in).

[jywarren]

I like this and was thinking a lot about it today, thanks for opening! Also came up in convo w @aashna27 -- what about automating the build somehow? Could we have a script to compile and bump the version on command? Are there any bots for this?

Thanks!!!

[jywarren]

Solving this would be very helpful for a number of public lab repositories! I wonder if there's a standard approach.

[VibhorCodecianGupta]

@jywarren Yes, precisely. I am working on this, trying to look for any automation or bots that automatically compile and bump. Will post a solution soon!

[rexagod]

Thank you so much for opening this issue @VibhorCodecianGupta!! This was on my mind too since I faced some conflicts myself and had to double check everything (especially the dist/) for changes before committing (which can be frustrating sometimes!). Like @jywarren said, this will definitely prove to be a critical addition to many repos! I do believe that the modular nature and mobility of the codebase should be one of our top priorities in any case. Obviously double checking and syncing the local fs with codebase every now and then can drastically reduce the chances of merge conflicts happening, but in case of first timers who aren't very experienced with git, these unnecessary conflicts can mean the loss of a potential long term contributor! Let me know if I can help you with anything.

Let's do this! :tada:

[VibhorCodecianGupta]

@jywarren @rexagod there is something very flowery in my mind, which is probably overkilling. A minimum functionality CLI wrapper could be written for admins which would bundle the files for serving. These files could go into a separate folder in the repository for third-party users to simply copy and paste. The current dist folder could be put into gitignore, because npm run start anyway builds and serves the file inside dist. The files served by the CLI tool would just be present for consumption by third-party users (or for whatever reason the dist files are not already ignored). The CLI commands could take a couple of arguments, including a secret and the semver, ensuring only project maintainers have access to those built files and modifying the semver.
This might be too much of a fix, but sounds cool. Thoughts? 😅

[harshkhandeparkar]

My idea would not be as overkill as Vibhor's but I'll try. I think only the source code should be available on the repository and the dist/ directory could be gitignored. Only the admins will release new updates through github releases page. For unstable daily/weekly builds we can have a separate beta branch? We can include the current dist in examples/lib/ or examples/dist/ for the example page or even a CDN like thing can be used. Maybe the build process can download the latest dist file using npm run setup.

[VibhorCodecianGupta]

@HarshKhandeparkar sounds good! Although, wouldn't putting the dist folder in examples/ again create conflicts? I'm not quite sure what you mean by example pages.

[harshkhandeparkar]

@VibhorCodecianGupta
Dist files in examples/dist/ are just optional they also should be ignored. They will be created when npm run setup is executed or can be downloaded manually. We can also include them from/dist/ when npm run setup executed. examples/dist/ can be used optionally. Its not really required.

I'm not quite sure what you mean by example pages.

By example page I mean the image-sequencer webpage in the examples/ directory.

[jywarren]

Hi, all - great brainstorming here. Thinking about a refined version of what you're talking about:

wrapper could be written for admins which would bundle the files for serving

We could make a Grunt task called grunt publish which:

1. runs tests
2. increments the package.json file by 0.0.1 (future version could prompt for major/minor/patch semver, maybe there are node modules for this too?)
3. compiles to /dist/
4. pushes to main/master with /dist/ and package.json updates
5. runs npm publish
6. merges to gh-pages for the online demo and pushes
7. pushes a new release with semver tagged branch to GitHub?

I think in this order, if any step fails, it'll stop, and steps 4-7 are already only allowed by maintainers/admins.

I think we need to keep providing the dist files in the repository so people can use them (in a browser context) by git pulling the repo without needing to have anything (grunt, node) installed.

How does this sound? If this works, we can do it about once per week, and if we really like it an it's relatively smooth, maybe we can automate it with Jenkins or something.

[jywarren]

Also curious to hear from @gauravano @SidharthBansal @sagarpreet-chadha @ryzokuken about this! I think it could significantly smooth out our PR workflow and as @rexagod says make it lower barrier for new contributors! Also it solves the issue with @dependabot not compiling in new dependencies!

[VibhorCodecianGupta]

@jywarren sounds really cool! I'd love to work on this :D

[jywarren]

This sounds great... would like to hear from a few more people but since the Grunt task should be relatively simple, perhaps you can get started?

Also, i just released all recent updates as v2.3.0 on npm and am also deploying it to gh-pages!

[jywarren]

For reference, when i deploy to gh-pages, we have to include the node modules, I run:

git checkout main
git pull publiclab main
git checkout gh-pages
git merge main -m 'merge from main'
npm install && npm update
git add node_modules
git commit -m 'updated node modules'
git push publiclab gh-pages
git checkout main
npm install

I don't know if this is ideal... i guess we should only be including node modules which aren't compiled in, but which are used for the examples? the folder is really gigantic.

[harshkhandeparkar]

I think we should download the required libraries like jQuery in the examples/ directory directly instead of using node_modules. This will also separate the library souce code from example implementation code.

[tech4GT]

Hmm, @jywarren I was thinking let's add a pre commit hook which builds and adds the dist files,

[rexagod]

And since .git/hooks are ignored, we can add an external script that generates the pre-commit hook and adds a symlink there, while obviously including the (external) hook in .gitignore :+1:

Edit: How about doing $ curl [hook link] > ./.git/hooks/pre-commit on, say, grunt add-pre-commit-hook and grunt.task.registerTask('add-pre-commit-hook', addhook) and grunt.task.registerTask('build', ['browserify:dist', 'add-pre-commit-hook']) so it'll check on grunt build?

We'll only need to change the link contents this way.

[jywarren]

This sounds great, although couldn't the hook also reference the gruntfile
task so it could be run independently?

In any case let's go ahead and prototype it. We can try it out and iron out
the issues to get this solved!

On Sat, Jan 5, 2019, 4:43 AM Pranshu Srivastava <[email protected]
wrote:

And since .git/hooks are ignored, we can add an external script that
generates the pre-commit hook and adds a symlink there, while obviously
including the hook in .gitignore 👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/601#issuecomment-451641673,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ-r2tu5cQWHTjShmyUecAGEf8ryIks5vAHOMgaJpZM4ZpEzv
.

[rexagod]

I think the catch here is creating something (./.git/hooks/pre-commit) that will not be locally present on on the users machine after cloning this repo. In any case, we need an external script (rename ./.git/hooks/pre-commit.sample by ./.git/hooks/pre-commit and add code inside) on master for doing that, after that we can use the hook to execute repititive grunt commands (like build) as @jywarren said.

Also it'll be nice if the hooks are written in node env, instead of git, since JS is way more common, just thinking out loud here.

[jywarren]

I wanted to highlight that pre commit hook which builds and adds the dist files while we should consider it because it'd be nice, it wouldn't solve the conflicts which so often come up and have to be rebased.

What if we made a task called grunt publish which:

1. compiles dist
2. publishes to gh_pages
3. runs npm publish

then we stop asking people to submit dist files?

[jywarren]

What do other folks think of this plan?

[jywarren]

This guide related to versioning but mentions that CI can be set up to add commits. Does this mean we could in theory build in builds to the Travis process??

https://threedots.tech/post/automatic-semantic-versioning-in-gitlab-ci/

[jywarren]

I wanted to bring this up again @publiclab/is-reviewers -- it's getting really tough to keep PRs rebased against the latest. I am more in favor now of just not asking PRs to include dist files, potentially with a .gitignore of dist, and then once every week or two, when we do a new NPM release, we build them all.

People would still do grunt build locally to test things out, but we would ask PRs to only include non-dist files. What do folks think?

Asking especially because i've run up against some really rough rebases recently!

[harshkhandeparkar]

I'm totally in favour of this. I do have another suggestion though. Is there a pre-merge hook? That can uglify the files on merge. They contributor can only browserify it. What do other folks think?
cc @publiclab/is-reviewers @jywarren

[jywarren]

I'm +1 on separating out the build into build and then uglify as separate steps. But we needn't do that as part of this PR, we can break that into it's own! Thanks!

[harshkhandeparkar]

What I meant was that we should only commit browserified and uglified dist should be gitignored.

[jywarren]

I think both should be left to a maintainer to do weekly, along with bumping the version number, so it shouldn't need to concern other contributors. But by separating the build/uglify tasks, we save a lot of time in building locally while developing. It takes a while!

[harshkhandeparkar]

Yes. Definitely. I think the gruntfile should be revised. Its very messy as of now. The ui dist files are not built in the build task. All the targets of a task are mentioned whereas the whole taskname can be used. Like instead of adding browserify:dist browserify:js uglify:dist uglify:js to the build task, browserify ulgify is enough. Etc. Can somebody open a new issue for that or do taht directly?
cc @jywarren @publiclab/is-reviewers

[harshkhandeparkar]

Discussion moved to #773