feat: integrate js-core into react-imgix #776

ericdeansanchez · 2021-04-01T18:38:58Z

The purpose of this PR is to integrate @imgix/js-core into
react-imgix. First, targetWidths has been reimplemented in terms of
the js-core client. Second, buildSrc has been implemented in terms of
ImgixClient.buildSrcSet. Lastly, a temp fix was made for the
Background component. Prior to this PR, the flow of the code/params
caused duplicate param key-value pairs to be written into query strings.

Ideally, we want to avoid getting into this kind of situation
altogether. However, doing that will require a bit more reworking,
refactoring, and (possibly || probably) breaking changes.

Closes #763

commit-lint · 2021-04-01T18:39:01Z

Features

use @imgix/js-core for buildURL and targetWidths (ce729bf)
impl buildSrc in terms of ImgixClient.buildSrcSet (7a018bd)

Styles

run pretty (1a2d410)
run pretty (b71c75e)
tidy up function ordering (a6dad2d)
remove unused import (7214ee8)
use switch statement to de-dup (99e8ca1)

Tests

buildSrc: js-core uses newline as delimeter, not space (f01b0ed)
Background: duplicate param keys are not allowed (4a13e0f)

Bug Fixes

temp fix to de-dupe h & w params in Background (576416d)

Code Refactoring

use object spread over assign (7ccce55)
create helpers, clean up (c443031)
use object spread over assign (104a2c5)
moving some furniture around (51a0864)
remove unused code (8d4cec6)
abstract building childprops into function (4bded78)
pull buildChildProps into constructUrl.js (9d2018d)
remove shouldComponentUpdate HOC (e9336ff)
remove shallowUntilTarget (9eb59ee)
extend React.PureComopnent to get shallow behavior (395aa01)
remove unused code (538b7cc)
gather and de-dup with buildChildProps (4ec8e0f)

Performance Improvements

use Object.assign over spread (72b1fd8)

Build System

increase karma timeouts (05778c8)

Contributors

ericdeansanchez

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

src/react-imgix.jsx

src/targetWidths.js

luqven

LGTM 👍🏼

src/constructUrl.js

src/react-imgix-bg.jsx

src/react-imgix.jsx

src/targetWidths.js

frederickfogerty

Overall I think this is really great and reduces the complexity of this library. Nice work! 🎉

DPR_QUALITY and DPR_QUALITY_VALUES should be able to be removed from constants.js because I think they're no longer used
I think the changes to use {...object} rather than Object.assign are nice stylistically, but they were introduced for performance reasons, and we shouldn't change these back unless we're sure that there is no change to performance. There was a 30% increase in performance and it would be a shame to lose these gains. Context: perf: remove object spreading #423
The title should be updated to not include a "."

package.json

src/HOCs/shouldComponentUpdateHOC.jsx

frederickfogerty · 2021-04-06T11:09:11Z

src/constructUrl.js

 import extractQueryParams from "./extractQueryParams";
+import { config } from "./common";
+const PACKAGE_VERSION = require("../package.json").version;


Let's change this to using common/constants.js, must have slipped through the cracks.

frederickfogerty · 2021-04-06T11:11:56Z

src/constructUrl.js

+  const [scheme, rest] = src.split("://");
+  const [domain, ...pathComponents] = rest.split("/");


An improvement here, if you want, could be to use the built-in URL interface to do this extraction. You know, cos, edge cases.

https://developer.mozilla.org/en-US/docs/Web/API/URL

Won't this add encoding to the path though? Or maybe I don't understand how I'd use the built-in interface?

Yeah I think you're right

frederickfogerty · 2021-04-06T11:16:36Z

src/constructUrl.js


+function compactParamKeys(longOptions, width, height) {


I don't think having width and height here as parameters is too coupled. I think it would be better to push this to the consumer with either:

const params = compactParamKeys({...longOptions, w: width, h: height});

or

const params = {...compactParamKeys(longOptions), w: width, h: height};

frederickfogerty · 2021-04-06T12:42:50Z

src/constructUrl.js

+  return /^\d+(\.\d+)?:\d+(\.\d+)?$/.test(aspectRatio);
+}
+
+function buildChildProps(obj, src, attributeConfig, width, height, refType) {


I think this function relates more to the components where it is called, so at least it should be located in that file.

It seems like a bit of a smell to be passing this around - seems like it could go wrong in the future if this function was to incorrectly assign to obj. I wonder if it's better if it's possible to make this pure, or just to deal with this logic being duplicated in the separate components. It's not super complicated so I'm not sure how much benefit there is to extract it into a common function.

Yeah, I think it smells a bit too, but I wanted to try to extract/distill some of the structure of what's going on here. I'll look into refactoring this.

src/constructUrl.js

frederickfogerty · 2021-04-06T12:46:33Z

src/react-imgix-bg.jsx

+    const allParams = { ...params, ...imgixParams, dpr };
    const srcOptions = {
-      ...params,
      fit: "crop",
-      ...imgixParams,
      ...(disableLibraryParam ? {} : { ixlib: `react-${PACKAGE_VERSION}` }),
+      ...allParams,


Is this the fix related to this comment?

Lastly, a temp fix was made for the Background component. Prior to this PR, the flow of the code/params caused duplicate param key-value pairs to be written into query strings.

frederickfogerty · 2021-04-06T12:50:33Z

src/targetWidths.js

-}
-
-export default targetWidths();
+export default ImgixClient.targetWidths();


Haha, nice! 👏

frederickfogerty · 2021-04-06T12:53:56Z

test/unit/react-imgix.test.jsx

+      expect(srcset.split(",\n")[0].split(" ")).toHaveLength(2);
+      const aSrcFromSrcSet = srcset.split(",\n")[0].split(" ")[0];
      expect(aSrcFromSrcSet).toContain(src);
-      const aWidthFromSrcSet = srcset.split(", ")[0].split(" ")[1];
+      const aWidthFromSrcSet = srcset.split(",\n")[0].split(" ")[1];


There's probably little value testing this stuff in react-imgix anymore since it's now implemented and tested by @imgix/js-core

I agree. I wanted to at least demo "hey, this works kind of like it did before." I wonder what work I could do to distill down the test suite now.

ericdeansanchez · 2021-04-14T00:36:45Z

Closing in favor of #780

ericdeansanchez added 6 commits March 25, 2021 16:25

feat: use @imgix/js-core for buildURL and targetWidths

ce729bf

style: run pretty

1a2d410

feat: impl buildSrc in terms of ImgixClient.buildSrcSet

7a018bd

test(buildSrc): js-core uses newline as delimeter, not space

f01b0ed

fix: temp fix to de-dupe h & w params in Background

576416d

test(Background): duplicate param keys are not allowed

4a13e0f

ericdeansanchez requested a review from a team as a code owner April 1, 2021 18:38

ericdeansanchez added 2 commits April 1, 2021 11:50

style: run pretty

b71c75e

Merge branch 'main' into integration

2bfd0b1

luqven reviewed Apr 2, 2021

View reviewed changes

src/react-imgix.jsx Outdated Show resolved Hide resolved

luqven reviewed Apr 2, 2021

View reviewed changes

src/targetWidths.js Show resolved Hide resolved

luqven approved these changes Apr 2, 2021

View reviewed changes

sherwinski reviewed Apr 2, 2021

View reviewed changes

src/constructUrl.js Show resolved Hide resolved

src/react-imgix-bg.jsx Show resolved Hide resolved

src/react-imgix.jsx Outdated Show resolved Hide resolved

src/react-imgix.jsx Outdated Show resolved Hide resolved

src/targetWidths.js Show resolved Hide resolved

ericdeansanchez added 10 commits April 2, 2021 14:33

refactor: use object spread over assign

7ccce55

refactor: create helpers, clean up

c443031

refactor: use object spread over assign

104a2c5

refactor: moving some furniture around

51a0864

refactor: remove unused code

8d4cec6

refactor: abstract building childprops into function

4bded78

refactor: pull buildChildProps into constructUrl.js

9d2018d

style: tidy up function ordering

a6dad2d

style: remove unused import

7214ee8

style: use switch statement to de-dup

99e8ca1

frederickfogerty suggested changes Apr 6, 2021

View reviewed changes

ericdeansanchez added 5 commits April 7, 2021 14:33

refactor: remove shouldComponentUpdate HOC

e9336ff

refactor: remove shallowUntilTarget

9eb59ee

refactor: extend React.PureComopnent to get shallow behavior

395aa01

refactor: remove unused code

538b7cc

refactor: gather and de-dup with buildChildProps

4ec8e0f

ericdeansanchez changed the title ~~feat: integrate js-core. into react-imgix~~ feat: integrate js-core into react-imgix Apr 9, 2021

ericdeansanchez added 2 commits April 12, 2021 13:41

perf: use Object.assign over spread

72b1fd8

build: increase karma timeouts

05778c8

ericdeansanchez closed this Apr 14, 2021

ericdeansanchez deleted the integration branch April 14, 2021 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate js-core into react-imgix #776

feat: integrate js-core into react-imgix #776

ericdeansanchez commented Apr 1, 2021 •

edited

Loading

commit-lint bot commented Apr 1, 2021 •

edited

Loading

luqven left a comment

frederickfogerty left a comment •

edited

Loading

frederickfogerty Apr 6, 2021

frederickfogerty Apr 6, 2021

ericdeansanchez Apr 13, 2021

frederickfogerty Apr 16, 2021

frederickfogerty Apr 6, 2021

frederickfogerty Apr 6, 2021

ericdeansanchez Apr 13, 2021

frederickfogerty Apr 6, 2021

frederickfogerty Apr 6, 2021

frederickfogerty Apr 6, 2021

ericdeansanchez Apr 6, 2021

ericdeansanchez commented Apr 14, 2021

		const [scheme, rest] = src.split("://");
		const [domain, ...pathComponents] = rest.split("/");

feat: integrate js-core into react-imgix #776

feat: integrate js-core into react-imgix #776

Conversation

ericdeansanchez commented Apr 1, 2021 • edited Loading

commit-lint bot commented Apr 1, 2021 • edited Loading

Features

Styles

Tests

Bug Fixes

Code Refactoring

Performance Improvements

Build System

Contributors

luqven left a comment

Choose a reason for hiding this comment

frederickfogerty left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdeansanchez commented Apr 14, 2021

ericdeansanchez commented Apr 1, 2021 •

edited

Loading

commit-lint bot commented Apr 1, 2021 •

edited

Loading

frederickfogerty left a comment •

edited

Loading