Create version flags

[jywarren]

Fixes #0000

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• PR is descriptively titled 📑 and links the original issue above 🔗
• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
• code is in uniquely-named feature branch and has no merge conflicts 📁
• screenshots/GIFs are attached 📎 in case of UI updation
• ask @publiclab/reviewers for help, in a comment below

[gitpod-io]

[stephaniequintana]

@jywarren
From your comment here,

I started the flag implementation in #438 - i will try to generate the dist files too -- but if that makes sense we can merge it and you can begin using if (options.version == 1) {} in your code!

This is great! Thank you for doing this. Question/thought:

• I noticed you added the flag to the dist/infragram.js file and not the dist/infragram2.js file... I assume that with the flag in place we can get rid of the infragram2.js file and now operate soley on infragram.js. Is this correct/the path we are taking? If so, should I open a PR that reflects this? (i.e. adding all of the version 2 functions from infragram2.js with the if (options.version == 2) {...} code to the infragram.js file and delete infragram2.js?)

• For clarity, I see that you did not add the version flag code, ( options.version = options.version || 1;) to the individual src files. Do we need to include it in those individual files that we are adding only version 2 functions to as well?

• I'd very much like to merge this so that we can begin adding the flag to our code. I'd also like to update Connected pre-loaded images to v2 #436 to include the flag.

@Forchapeatl, what do you think? Do you see any issues with merging this and including the flags into our code moving forward?

[jywarren]

I assume that with the flag in place we can get rid of the infragram2.js file and now operate soley on infragram.js. Is this correct/the path we are taking?

Yes, that should be right!

If so, should I open a PR that reflects this? (i.e. adding all of the version 2 functions from infragram2.js with the if (options.version == 2) {...} code to the infragram.js file and delete infragram2.js?)

You can, but we don't have to do it all in one pass. We can migrate anything in infragram2.js into infragram.js one step at a time, with the goal of a single file. But doing it all up front could be complex so there's not a huge rush.

For clarity, I see that you did not add the version flag code, ( options.version = options.version || 1;) to the individual src files. Do we need to include it in those individual files that we are adding only version 2 functions to as well?

Which files did you think it should be in? Maybe I missed something!

I'd very much like to merge this so that we can begin adding the flag to our code. I'd also like to update #436 to include the flag.

Agreed, let's merge this as soon as i see what you mean in "For clarity, I see that you did not add the version flag code...", and then other work can be rebased on top. Thanks!

[stephaniequintana]

@jywarren,

We can migrate anything in infragram2.js into infragram.js one step at a time, with the goal of a single file. But doing it all up front could be complex so there's not a huge rush.

Ok, this sounds good - I also think its best to do this in smaller chunks. For now, index2.html (version 2) in connected to infragram2.js. I will leave this as is until we move everything over and simply add any new functions to both the infragram.js and the infragram2.js files

Can you please clarify; would you like us to utilize the version 2 flag on all functions that relate to version 2 or simply those that would break version 1? For example, #436 does not break version 1, (but also does not include the flag).

For clarity, I see that you did not add the version flag code...",

You added the flag to the src/Infragram.js file. I was just wondering if we needed to add it to the src/ui/interface.js file (and other src files that we are adding-to/changing). Infragram.js requires those modules, I just wasn't clear on whether that is sufficient. I actually think we're covered and can change anything later if need be 👍

[jywarren]

Oops, my apologies for missing this --

Can you please clarify; would you like us to utilize the version 2 flag on all functions that relate to version 2 or simply those that would break version 1? For example, #436 does not break version 1, (but also does not include the flag).

I think only when necessary - that is, when not using the flag would break v1. Many additions simply address HTML bits that don't exist in v1, so they won't break anything if included. However, I think that it's good to mark such additions with a comment noting that they are for v2, so that we know what we can clean up later when we remove v1 code!

[jywarren]

You added the flag to the src/Infragram.js file. I was just wondering if we needed to add it to the src/ui/interface.js file (and other src files that we are adding-to/changing). Infragram.js requires those modules, I just wasn't clear on whether that is sufficient. I actually think we're covered and can change anything later if need be 👍

Well, once we include that flag in the main options parameter, it's accessible from most of the sub-files. See how we pass in the options object here:

infragram/src/Infragram.js

Line 5 in 86b2ddb

options.camera = require('./io/camera')(options);

Then it's accessed within the file here:

infragram/src/io/camera.js

Line 3 in 86b2ddb

module.exports = function Camera(options) {

And you can see us using it here:

infragram/src/io/camera.js

Line 111 in 86b2ddb

options.processor.updateImage(video);

Any subfile in /src/ which you see with the same options object passed through works in the same way, so that all options are consolidated. Does that make sense?

[jywarren]

Hi @Forchapeatl @stephaniequintana I just merged this - so you can now access it like: if (options.version == 2) in most files so you can switch between. Have you two been able to connect about how we'll aim to consolidate all functions in the same infragram.js file, and to use this flag to ensure v1 doesn't break when we add features that only work in v2? Thank you!

[stephaniequintana]

It does make sense. Thanks for clarifying 👍

[stephaniequintana]

We haven’t discussed this specifically yet.

With this merge, if we can include the flag on any outstanding and new PRs (where necessary), there should only be a few instances that we’ll need to address.

To help keep things organized, I propose that we do this after we merge @Forchapeatl’s latest work into version 2. I’ll touch base with @Forchapeatl and add this to our running thread.

[jywarren]

With this merge, if we can include the flag on any outstanding and new PRs (where necessary), there should only be a few instances that we’ll need to address.

Great!

To help keep things organized, I propose that we do this after we merge @Forchapeatl’s latest work into version 2. I’ll touch base with @Forchapeatl and add this to our running thread.

Sounds excellent.