Multiple copies of alameda.js in the codebase

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: kgrandon, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
There are at least three applications which use alameda.js. We should abstract this into the shared/ folder at a minimum for now.

./apps/camera/js/vendor/alameda.js
./apps/clock/js/alameda.js
./apps/email/js/alameda.js
./apps/system/camera/js/vendor/alameda.js
./build_stage/camera/js/vendor/alameda.js
./build_stage/clock/js/alameda.js
(Reporter)

Comment 1

5 years ago
Created attachment 8358841 [details] [review]
Pull Request - Add shared alameda.js

Hi David, Vivien - Wondering if either of you guys could review this. If we land this, we can land individual patches per app to leverage the single copy of alameda.js. Thanks!
Attachment #8358841 - Flags: review?(dflanagan)
Attachment #8358841 - Flags: review?(21)
(Reporter)

Comment 2

5 years ago
Created attachment 8358842 [details] [review]
Pull Request - [Clock] Use shared alameda.js
The build_stage locations are build artifacts, so should be ignored for determining the uses of the file.

I suggest instead of using /shared that consumers of alameda just pull the versions they need from the alameda repo then document what version they use in an app package.json. volo (http://volojs.org) can be used to do this via a tool approach vs a manual approach.

Note that while volojs is like a package manager, it does not place any burden on the rest of the tooling for gaia: no travis issues or possible issues like npm being down. It is expected that the dependencies are committed with the source tree for the app. The manual approach of just logging the version in the package.json and updating the file in the app source tree is also sufficient.

I prefer this over the /shared location because /shared forces upgrades on multiple apps whenever /shared is updated, and I think it is more prudent for the individual apps to decide to rev the version when they feel they need any of the changes.

The /shared directory does not help for runtime as each app gets a copy of the shared resources for the app deployment. It is just a holding area for commonly used libs for gaia. That holding area instead could be separate github repos for the shared resources that are needed.

For alameda in particular, it has a life of its own outside of gaia, with its own canonical location in github already, so there is less of a case for keeping it in /shared.

For an app to use volo for alameda:

I will use the email app as an example, but replace "email" with the target app name:

1) Create apps/email/package.json with the following content:

{
  "volo": {
    "baseUrl": "js"
  }
}

2) Install alameda by running this command in the apps/email directory:

    volo add requirejs/alameda

This will install the latest version tagged alameda at apps/email/js/alameda.js and the package.json after this command will look something like this:

{
  "volo": {
    "baseUrl": "js",
    "dependencies": {
      "alameda": "github:requirejs/alameda/0.1.0"
    }
  }
}

Specific commits can be installed via `volo add requirejs/alameda/[commit-hash-here] More details on the types of dependency identifiers:

https://github.com/volojs/volo/blob/master/commands/add/doc.md

`volo update` can be used later to update a dependency when the app maintainer feels it is appropriate.

Related dev-gaia post:
https://groups.google.com/d/msg/mozilla.dev.gaia/D4kRaSDspQI/eSumBYn0y-sJ
Comment on attachment 8358841 [details] [review]
Pull Request - Add shared alameda.js

I'm fine with landing that into shared/ so feedback+. I would trust jburke for the review.
Attachment #8358841 - Flags: review?(jrburke)
Attachment #8358841 - Flags: review?(21)
Attachment #8358841 - Flags: feedback+
Comment on attachment 8358841 [details] [review]
Pull Request - Add shared alameda.js

As mentioned in Comment 3, I prefer that individual apps have the ability to upgrade the version when they feel they want a newer version, and tracking where they got their version in the app's area. Particularly since alameda already has its own repo, and apps ship with their own copy of the file once app deployment is done.

However I am still new to design goals around /shared and may be missing something, so open to learning more in that case.
Attachment #8358841 - Flags: review?(jrburke)
(Reporter)

Comment 6

5 years ago
Comment on attachment 8358841 [details] [review]
Pull Request - Add shared alameda.js

(In reply to James Burke [:jrburke] from comment #5)
> Comment on attachment 8358841 [details] [review]
> Pull Request - Add shared alameda.js
> 
> As mentioned in Comment 3, I prefer that individual apps have the ability to
> upgrade the version when they feel they want a newer version, and tracking
> where they got their version in the app's area. Particularly since alameda
> already has its own repo, and apps ship with their own copy of the file once
> app deployment is done.

Hi James - My thoughts are that a proper dependency management solution is necessary for gaia shared code. Unfortunately I think this is a fairly large project, involving several teams - for example we would need to work with releng for whatever solution we choose (besides repo). The proper solution is likely many months away, but I do think we can improve our codebase significantly in the interim. Removing duplicate libraries and consolidating in shared/ is a good first step, and likely something we'll be doing for things like formatters (bug 934025) and event emitters (bug 958864).

Regarding upgrades, I think that we can safeguard ourselves against mishaps by only upgrading shared libraries fairly early in the process, and ensuring our code is well tested. Obviously this is does not guarantee anything, but if we take upgrades seriously, and only do them at the beginning of the cycle, I would feel fairly confident in our ability to prevent regressions.

I think that moving this into shared/ would be much better than not doing anything, as we are likely months away from being able to use volo or some other solution. I'm opening this up for review again. If you feel that it is an ok intermediate step - let's land it. If not, I'll work on bringing other libraries into shared/ and we can revisit this sometime in the future.
Attachment #8358841 - Flags: review?(dflanagan) → review?(jrburke)
Comment on attachment 8358841 [details] [review]
Pull Request - Add shared alameda.js

Copying the desired version into the repo meshes with how front end projects have managed JS dependencies before, and I expect it to be true even in the future. The developer does not have to use volo, it was just a tool to make that dev-time upgrade easier within an app. A manual copy (as done today) is fine too.

Depending on a tool that fetches dependencies on-demand over the network to set up the app, and also possibly a whole version of the dependency repo (some with tests!), is really wasteful and prone to downtimes/network conditions. Just grab the version desired and commit to the app. Front end JS dependencies are frequently just one JS file. Front end JS also does not have the compiling issue that makes on-demand repo/npm fetches more necessary.

I do not expect some bulk conversion of gaia apps to a "consume shared resources from other repos/registries with tooling" to happen overnight. That requires way too much agreement, as you mention. One way to start gradually is to put any new shared/ resources, particularly the single file JS files, in their own repos as they come up and start the pattern of apps grabbing the versions they want and placing them in the app repo.

Alameda seemed like a good place to try this since it is already developed in a separate repo, and as mentioned, I only see a downside with placing it in /shared. However, you and others have been around longer, and may know of other positives for placing it in /shared, so it would be helpful for me to learn about them.

While it is possible to work through the bugs by using /shared and upgrading early in the cycle, that is creating extra work that can be avoided until it makes sense for the individual apps. They may even be able to skip a few versions if they are fine with the current one they are using.

Plus, by using app-specific copies of versioned releases we get to try out a likely end point for how apps and their dependencies will work in the future.

That said, if there are other reasons to use /shared that I am unaware of, please let me know so I learn, and I will defer to project norms. I do not like the restriction it places on me being able to upgrade or try new versions within an app, and I can see an app later checking in their own app-local version (or even a completely different AMD loader) if late in the cycle and they do not want to impact other teams.

If you do go ahead with a merge, I suggest using alameda 0.2.0 as it is the latest and its promise implementation matches latest promises a+ specs, and I have pull requests for email (bug 958675) to update to it:
https://raw.github.com/requirejs/alameda/0.2.0/alameda.js

Clearing review based on the above. While I am not r-, I am not r+ either, for longer range reasons, and would prefer to move in that direction. However, I am fine if you do merge though and I can update email if you did do.
Attachment #8358841 - Flags: review?(jrburke)
(Reporter)

Comment 8

5 years ago
Thanks for the points James. I think we are in the process of moving lots of libraries into shared/. I'll hold off for now and we can see what the landscape looks like in the future.
(Reporter)

Updated

5 years ago
Assignee: kgrandon → nobody
Each time we have an issue with "no space to flash gaia on the phone" is a case where we should have this fixed.
(Reporter)

Comment 10

4 years ago
(In reply to Hubert Figuiere [:hub] from comment #9)
> Each time we have an issue with "no space to flash gaia on the phone" is a
> case where we should have this fixed.

Unfortunately this wouldn't solve that case. We currently copy required shared files into each application. The point of this bug was to remove duplicate code from each application, as it's generally a good practice to not have duplicate code.

It would be good to do a shared/ resource audit of apps though to see how much space we could actually save.
It would be a starting point. :-)

Comment 12

4 years ago
add ./apps/settings/js/vendor/alameda.js in list. 

I think gaia as a whole we should keep our library simple and avoid use different versions.
(Reporter)

Comment 13

4 years ago
(In reply to Fred Lin [:gasolin] from comment #12)
> add ./apps/settings/js/vendor/alameda.js in list. 
> 
> I think gaia as a whole we should keep our library simple and avoid use
> different versions.

I agree, but I got lots of pushback on this in the past. Mainly I'm just waiting for ES6 modules right now, whenever they may come, but if there is a desire to put alameda.js into shared/ and consensus amongst app owners I will gladly help.

Comment 14

4 years ago
David, Marcus, James, Arthur, (camera, clock, email, settings)

Is there still any concern that NOT to put alameda.js in shared/?

Since from system platform we did not have any plan to change to other build solution at this time.)
Flags: needinfo?(m)
Flags: needinfo?(jrburke)
Flags: needinfo?(dflanagan)
Flags: needinfo?(arthur.chen)
See comment 3 and comment 7, but to summarize:

* alameda already has its own repo. It does not need the /shared space to track its development.

* gaia apps should get used to pull in specific versions/changeset IDs of dependencies vs getting pushed whatever is in /shared. That is usually how broader web development is done, with each webapp pulling the version/hash of the dependency it wants, and I believe that is the model being tried for some of the gaia web components.

* Because of the push model with /shared, I do not want to be responsible for profiling and possibly having to fix other apps if I update the version of alameda that I use in my app. In bug 1005446, I may land a version of alameda that uses native promises instead of its own promise polyfill. However, native promises are currently clocking slower than what is possible with a promise polyfill. For email, we may be OK with that tradeoff, but I think each project should make their own decision about that.

If some gaia apps prefer to use a /shared copy if that works for them, then go for it, but in that case, do not update the email app to use it. 

Email will continue to reference its own version, and when I update email, I will just be updating the version that email uses in the email app dir. I will send email to dev-gaia when I do change it so that others are aware of newer alameda versions, but I do not plan on force updating the other apps.
Flags: needinfo?(jrburke)
(Reporter)

Comment 16

4 years ago
(In reply to James Burke [:jrburke] from comment #15)
> See comment 3 and comment 7, but to summarize:
>
> * Because of the push model with /shared, I do not want to be responsible
> for profiling and possibly having to fix other apps if I update the version
> of alameda that I use in my app. In bug 1005446, I may land a version of
> alameda that uses native promises instead of its own promise polyfill.
> However, native promises are currently clocking slower than what is possible
> with a promise polyfill. For email, we may be OK with that tradeoff, but I
> think each project should make their own decision about that.

This is exactly the opposite of what we need to fix across gaia. We need to have a set of performance best practices and stick to them across all apps. We need to come to these decisions as a group, and can't have apps going off and doing their own thing. If we don't, that's when we'll have a painful time profiling.
(In reply to Fred Lin [:gasolin] from comment #14)
> David, Marcus, James, Arthur, (camera, clock, email, settings)
> 
> Is there still any concern that NOT to put alameda.js in shared/?
> 
> Since from system platform we did not have any plan to change to other build
> solution at this time.)

I don't use alameda in my apps. It is fine with me to land alameda.js in shared/js if someone needs it there. But what I hear from the camera developers matches what James has been saying here... Developers who want to use alameda.js in their apps are likely to also want to use Bower or something similar (Is that what Volo is?) to pull dependencies into their app and would not want to use the shared copy of alameda, preferring instead to pull a copy from the upstream alameda repo. I'm almost certain that the camera developers will not use a shared copy of alameda. I can't speak for the other apps.
Flags: needinfo?(dflanagan)
Can we just do some SHA-comparisons during the build step and symlink identical files? It sucks to have duplicated space on disk, but lockstepping non-user-visible library versions is a poor solution in the general case, and will make updates more difficult down the road.
Flags: needinfo?(m)
:mcav - as I understand it, the apps are delivered as zip files onto the device. Not sure how that would work with the symlinking. In my dreams, email updates could be delivered from the marketplace, so that might add more complication. Feel free to file a separate bug though to float that as a general idea.
(In reply to James Burke [:jrburke] from comment #19)
> :mcav - as I understand it, the apps are delivered as zip files onto the
> device.

Huh, didn't realize we actually keep them stored as zipfiles on the device, rather than extracting them upon install. If that's the case, then the space is probably negligible anyway.
Apps do not get benefit in size by using shared resources as we always have a copy of the required resources in each app. Using shared resources also makes the app unable to control the version of the resource it uses. Given the facts settings app would use its own copy of alameda.js.
Flags: needinfo?(arthur.chen)
(Reporter)

Comment 22

4 years ago
There's igger fish to worry about than this, and if app owners don't want it, let's not force it.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.