thegreenwebfoundation / co2.js

An npm module for accessing the green web API, and estimating the carbon emissions from using digital services
Other
389 stars 49 forks source link

Ensure OneByte model is used as default (for now) #99

Closed fershad closed 2 years ago

fershad commented 2 years ago

This PR ensures that the OneByte model is used as default when no model parameter is passed to the CO2 class.

mrchrisadams commented 2 years ago

Hey buddy - I don't think this PR is necessary - your code was fine in the first place.

I checked out the co2.js code locally today, and tried to reproduce the behaviour we saw, and also tested with the code had been published to npm.

What I think might have happened is that work from another branch, probably v0.11.x, had been transpiled/compiled and was in already in the dist folder when an npm publish took place.

I'm basing this on this snippet of the 'compiled' code in dist for co2.js, when I pulled down 0.10.3

import OneByte from "./1byte.js";
import SustainableWebDesign from "./sustainable-web-design.js";
class CO2 {
  constructor(options) {
    this.model = new SustainableWebDesign();
    if ((options == null ? void 0 : options.model) === "1byte") {
      this.model = new OneByte();
    }
  }
  perByte(bytes, green) {
    return this.model.perByte(bytes, green);
  }
  perVisit(bytes, green) {
    var _a;
    if ((_a = this.model) == null ? void 0 : _a.perVisit) {
      return this.model.perVisit(bytes, green);
    } else {
      console.warn("The model you have selected does not support perVisit. Using perByte instead.");
      return this.model.perByte(bytes, green);
    }
  }
// snip
}

Here's what I think happened.

Because npm publishes the contents of the dist folder, and we have a few builds to support different kinds of runtime, if you work in one branch, run a build step, and then go back to main to publish something to npm, the output of the build step on the previous branch can still be in the dist folder.

So, even if the contents ofsrc is the main branch the contents of dist can be from the earlier branch we were working on. We need to clear the compiled code, and make sure we have the latest build before we publish.

This looks like an easy mistake to make in future, so I've added a new command, npm release:patch to reduce the chances of us publishing the wrong build, as it took a while for me to figure out what was going on.

Would you please close this once you've read over this comment?

It's possible to trigger publishing as part of CI, and that would be the ideal scenario, but I'd appreciate your thoughts on how best we might to do this in future.

fershad commented 2 years ago

Today I learnt.

Cheers Chris, thanks for explaining that! Will be sure to keep this in mind when publishing in the future (even if we do automate steps).