Skip to content

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Nov 28, 2018

WHY are these changes introduced?

Move us closer to enabling the import/no-cycle rule by fixing several violations.

We can't turn it on just yet, as there's still a few more violations, but I want to chat with @danrosenthal about the best fix for those first.

WHAT is this pull request doing?

Creates per-component types.ts files to contain types in cases that result in cyclical dependencies.

E.g. Consider the following:

// In Component.ts
import {bar} from './utilities.ts';

export interface ThingType {
  x: string;
}

const x = bar({x:'foo'});


// In utilities.ts
import {ThingType} from './Component.ts';

export function bar(thing: ThingType):  {
  return thing.x;
}

In this case Component imports utilities and utilities imports Component - A cyclical dependency. To fix this, introduce a new file that contains the type, and have Component and utilities import from that file:

// In Component.ts
import {ThingType} from './types.ts';
import {bar} from './utilities.ts';

const x = bar({x:'foo'});


// In utilities.ts
import {ThingType} from './types.ts';

export function bar(thing: ThingType):  {
  return thing.x;
}

// In types.ts
export interface ThingType {
  x: string;
}

Now, utilities depends on types, and Component depends on utilities and types. The cycle is removed.

How to 🎩

  • Run tests
  • Run yarn run sk type-check to prove typescript is happy
  • Edit .eslintrc and remove the "import/no-cycle": "off", line and see that the number of warnings is reduced compared to master (should be 26 on master to 14 8 on this branch)

Move us closer to enabling the `import/no-cycle` rule by fixing several
violations.
@BPScott BPScott temporarily deployed to polaris-react-pr-691 November 28, 2018 23:13 Inactive
@BPScott BPScott added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Nov 28, 2018
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@BPScott BPScott merged commit f364dc6 into master Nov 29, 2018
@BPScott BPScott deleted the remove-cyclical-types branch November 29, 2018 17:54
@danrosenthal danrosenthal temporarily deployed to production December 4, 2018 19:16 Inactive
dleroux added a commit that referenced this pull request Dec 4, 2018
This reverts commit f364dc6, reversing
changes made to 8c2cb7a.
dleroux added a commit that referenced this pull request Dec 4, 2018
This reverts commit f364dc6, reversing
changes made to 8c2cb7a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants