imgix / js-core

A JavaScript client library for generating image URLs with imgix
https://www.imgix.com
BSD 2-Clause "Simplified" License
122 stars 21 forks source link

refactor!: access settings through setters and getters #342

Closed luqven closed 1 year ago

luqven commented 2 years ago

Description

This PR demonstrates how we could refactor js-core to use setters/getters to allow us to read and modify this.settings at any time during the object's lifecycle.

The proposal is to add a get and set object property binding to each settings we'd like to be able to modify outside the constructor.

// we'd like to be able to do this
const client = new ImgixClient({domain: 'test.imgix.net'})
client.domain = 'sdk-test.imgix.net`
// so in the class we do something like this
export default class ImgixClient {
  constructor(opts = {}) {
    this._settings = { ...DEFAULT_OPTIONS, ...opts };
    // ...
  }

  get domain() {
    return this._settings.domain;
  }

  set domain(value) {
    if (typeof value != 'string') {
      throw new Error('ImgixClient must be passed a valid string domain');
    }

    if (DOMAIN_REGEX.exec(value) == null) {
      throw new Error(
        'Domain must be passed in as fully-qualified ' +
          'domain name and should not include a protocol or any path ' +
          'element, i.e. "example.imgix.net".',
      );
    }
    this._settings.domain = value;
  }

Note: the properties set using the getters/setters will be set on the prototype of the class and not the instance.

Before this PR

You could not change certain settings, ie libraryParam outside of the constructor without running into issues where this.settings was not in sync with this.libraryParam.

After

You can set includeLIbraryParam ~and libraryParam~ using a setter.

Checklist

commit-lint[bot] commented 2 years ago

Code Refactoring

Tests

Features

Chore

Contributors

luqven

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
luqven commented 1 year ago

Closing this as it's gone stale. We wiill revisit it some other time.