Remove side-effects from spaces #184

sgomes · 2022-06-30T12:00:47Z

Following the discussion in #163 and #183, this (starts to) implement an approach laid out by @LeaVerou to remove the usage of side-effects in spaces, and make them available externally in a tree-shakeable way:

I think probably the best way forwards is:

Color spaces drop side effects

spaces/index-fn.js imports and re-exports all of them

index-fn.js imports and re-exports spaces/index-fn

spaces/index.js imports everything from spaces/index-fn.js and registers it calling ColorSpace.register()

Please let me know what you think!

Aside: my editor seems to be adding newlines at the end of files. Please let me know whether I should change my config to stop that from happening!

src/spaces/index.js

LeaVerou · 2022-06-30T12:48:01Z

Aside: my editor seems to be adding newlines at the end of files. Please let me know whether I should change my config to stop that from happening!

I'm fine with that!

package.json

src/spaces/index.js

LeaVerou · 2022-06-30T12:50:55Z

Looking good so far!

LeaVerou · 2022-06-30T14:32:59Z

src/space.js

+
+		if (this.aliases) {
+			for (let alias of this.aliases) {
+				this.registry[alias] = space;


The id for the color() function should be decoupled from aliases. In the case of XYZ it happens that the alias is also an id for color(), but not sure this should be enforced by the API

(ignore any previous comments, I had misread)

Oh, right, that's a good point, thank you! Sounds like space aliases need to be separate from color ID aliases 👍

Also, what should be the expected behavior if one registers two spaces with the same id or alias? Right now each one will overwrite the previous one.

Throwing a runtime error seems like the best bet to me. Having a unit test that checks to make sure there aren't any such errors on the full lib should then be enough to make sure the build doesn't break.

How does that sound?

I added a proposed runtime validation mechanism to 2a6176b, please let me know what you think!

Oh, and please feel free to bikeshed things like idAliases, as I'm not sure that's the best name 🙂

Not sure about a runtime error, users should be able to call register() multiple times from different parts of the code to register the same color space. Though perhaps we could only throw the error if the color space object is different.

Yes, I totally agree that it shouldn't error out if registering the same space multiple times. The code should be doing that already, but do let me know if I missed something!

package.json

src/space.js

LeaVerou · 2022-06-30T16:38:24Z

src/space.js

+			}
+		}
+
+		this.validateIds();


I like detailed errors as much as the next person, but checking that deeply seems a little too much. I'd skip this.

Do you mean we should forgo the ID check altogether? I think that's okay. As far as I can tell, if there is a collision, it'll simply use the first match, which is probably an acceptable behaviour?

LeaVerou · 2022-06-30T16:47:14Z

src/space.js

-		this.registry[id] = space;
-		return space;
-	}
+		this.addToRegistry(id, space);


Do we need a separate method? When somebody is using the register(id, space) signature, we should not be registering any additional things like aliases. So all we need to do is preserve what signature was used. Then we can recursively call it to register a color space with each alias, and still only have the checking code in one place.

Sorry, I'm not entirely sure I understand 😕

Do you mean that we should't have the aliases as part of the color space definition (space.aliases), and we should instead delegate the responsibility of registering the alias to the consumer? Or do you mean that we should simply call register recursively from within itself, and keep the responsibility of registering aliases within the ColorSpace class?

I mean, there is a semantic difference between the two signatures. If someone calls ColorSpace.register("foobar", mySpace) they intend to register mySpace under the "foobar" alias, and not do any other registration. E.g. someone may import a color space and want to register it with a different id altogether. If they do ColorSpace.register(mySpace), then it makes sense to register it with its id and aliases.

This also means that we can use it that way internally: When ColorSpace.register() is used with a single argument, we can internally call ColorSpace.register(id, space) as well as ColorSpace.register(alias, space) for every alias, thus not needing a separate function for this.

Does it make sense now?

Oh, I see, thank you for clarifying that! Yes, that sounds like a good approach 👍

LeaVerou · 2022-07-01T11:43:49Z

src/spaces/xyz-d65.js

@@ -10,8 +10,9 @@ export default new ColorSpace({
 	},
 	white: "D65",
 	formats: {
-		color: {}
+		color: {
+			ids: ['xyz'],


We should probably add both, for clarity

package.json

src/spaces/index.js

sgomes · 2022-07-04T11:51:40Z

Do you think this is ready to move forward to full implementation across all color spaces, @LeaVerou?

LeaVerou · 2022-07-04T12:20:56Z

src/spaces/index.js

+}
+
+// ...


Don't forget to re-export!

Oh, do we want to re-export from this file as well? No problem, can do 🙂

Yes, as people who want side effects might still need a reference to one of the color spaces (and it costs nothing to re-export so why not).

LeaVerou · 2022-07-04T12:21:22Z

Yes! 😄

sgomes · 2022-07-04T13:39:05Z

I've applied this to all color spaces, and rebased on main. I think this is ready for review!

sgomes · 2022-07-04T13:40:55Z

Also, please let me know if there's anything missing with regards to documentation, testing, and the like!

LeaVerou · 2022-07-04T14:00:10Z

Thank you! The testsuite is in a state of disarray right now, but have you checked that all passing tests still pass? (I just pushed some fixes so you'd need to rebase first)

sgomes · 2022-07-04T14:17:51Z

Thank you! The testsuite is in a state of disarray right now, but have you checked that all passing tests still pass? (I just pushed some fixes so you'd need to rebase first)

I did try to run the tests, but even after rebasing from main again I'm still getting 404s on resources from raw.githubusercontent.com. Perhaps I'm missing something? I'm not entirely sure how to actually go about running the tests; all I tried was to open tests/index.html locally.

LeaVerou · 2022-07-04T15:51:24Z

Thank you! The testsuite is in a state of disarray right now, but have you checked that all passing tests still pass? (I just pushed some fixes so you'd need to rebase first)

I did try to run the tests, but even after rebasing from main again I'm still getting 404s on resources from raw.githubusercontent.com. Perhaps I'm missing something? I'm not entirely sure how to actually go about running the tests; all I tried was to open tests/index.html locally.

Oh shit, you're right, the test harness is broken for users who are not logged in to GitHub. I'm on it, but it will take a while. Meanwhile you can try loading the different HTML pages individually (or log in). Sorry this is such a PITA. 😢

sgomes · 2022-07-04T16:38:17Z

Oh shit, you're right, the test harness is broken for users who are not logged in to GitHub. I'm on it, but it will take a while. Meanwhile you can try loading the different HTML pages individually (or log in). Sorry this is such a PITA. 😢

No worries at all 🙂

Looks like there are some issues to figure out on my end, namely with ordering; registering xyz-abs-d65 before xyz-d65 results in an error, because the former depends on the latter. We may have to go back to explicitly listing them (instead of naively looping over them) to maintain ordering, or instead being smarter about it and reordering at registration time.

LeaVerou · 2022-07-04T17:50:01Z

Wait, does xyz-abs-d65 refer to xyz-d65 by id? It should import the color space and use that reference.

sgomes · 2022-07-05T09:35:24Z

Wait, does xyz-abs-d65 refer to xyz-d65 by id? It should import the color space and use that reference.

The issue isn't something that xyz-abs-d65 specifically is doing, but rather the ColorSpace constructor:

this.base = options.base ? ColorSpace.get(options.base) : null;

Since base is provided through an ID, and since ColorSpace.get relies on the color space registry, this fails during construction of the xyz-abs-d65 space.

One way of solving this would be by having base provide not the ID but the ColorSpace instance itself. What do you think, @LeaVerou?

LeaVerou · 2022-07-05T11:45:20Z

Hi Sergio, yay! Also, the testsuite should be working now. Could you please check that the same number of tests passes before and after your changes?

sgomes · 2022-07-05T11:56:28Z

I rebased and tested main vs this branch, and I get the exact same number of passes/failures for all tests 👍

Some of the tests still fail with 404s, this time from projects.verou.me, but everything seems to be the same before and after these changes.

LeaVerou · 2022-07-05T12:46:08Z

I just fixed the tests to eliminate the 404, but the failing requests were for cosmetic things, so I don't think this affects anything here. I think I'm gonna go ahead and merge this! Thanks again!

sgomes · 2022-07-05T12:50:31Z

Woohoo, thank you @LeaVerou! 🎉

After these changes, the procedural API is looking quite good. To give you an example, here is a map for what gets included on a simple test case that just makes use of parse, serialize, ColorSpace (for registration), and the sRGB color space:

21 KB uncompressed / 8.4KB compressed is quite good, especially considering all the keywords are in there 🙂

LeaVerou · 2022-07-05T13:07:21Z

This should be in the upcoming v0.3.0 release, hopefully later today, but failing that tomorrow.

LeaVerou · 2022-07-05T13:54:00Z

Btw what did you use to generate the graphic above? It's really cool!

sgomes · 2022-07-05T14:04:51Z

Btw what did you use to generate the graphic above? It's really cool!

It's the webpack-bundle-analyzer plugin. It's really handy, I use it all the time! 🙂

I created a little test project, set up webpack on it, and added the plugin to the webpack config. The only thing to note is making sure to disable module concatenation, otherwise it'll all be mangled up into one giant index.js.

Here's the full webpack config, if that helps!

const path = require('path');
const BundleAnalyzerPlugin = require('webpack-bundle-analyzer').BundleAnalyzerPlugin;

module.exports = {
  entry: './src/index.js',
  output: {
    filename: 'main.js',
    path: path.resolve(__dirname, 'dist'),
  },
  plugins: [
    new BundleAnalyzerPlugin()
  ],
  optimization: {
    concatenateModules: false
  }
};

sgomes requested a review from LeaVerou June 30, 2022 12:00

sgomes commented Jun 30, 2022

View reviewed changes

src/spaces/index.js Outdated Show resolved Hide resolved