Skip to content

Conversation

@BPScott
Copy link
Member

@BPScott BPScott commented Nov 25, 2021

WHY are these changes introduced?

Hitting two birds with one PR - removing a confusing way of importing files, and a chonky test speed improvement.

components is usable as an alias for importing the components in tests. However feel it should be removed as it is usage is not intuitive and has the potential to accidentally break consumers:

  • It is possible to use the alias to import a type in source files, and TS will pass but when the package is published consumers will get an error
  • Importing concrete values (i.e. not types) fails in source code but works in test files. This is confusing and unintuitive to folks unfamiliar with the codebase.

In addition, using this file to access components means that Jest has to read and parse all components (as that file reexports every component) even if you only use a specific component or two in a given test. This slows tests down as jest is doing lots unneeded work. This gets particularly pathological when you're trying to run a single test or when you're running tests with a cold cache (i.e on CI).

In order to ensure that even relative path imports to the components index never reappear we should remove the components index file, per the recommendations and investigations in this web decision doc that spurned the investigation in this PR

WHAT is this pull request doing?

This PR replaces all instances of import {BLAH} from 'components' with import {BLAH} from '../../BLAH' or whatever is the correct relative path to that component is.

loom.config.ts and tsconfig.json have been updated to remove the ability to use 'components' as an importable path

How to 🎩

Tests / type-check passes

Stats!

I mentioned this improves performance on cold-cache test runs on CI.

Running 3 ci-style cold test runs rm -rf .loom && CI=1 yarn test on main takes: 50.01s, 50.30s, 49.04s.
Running 3 the same on this branch takes: 32.57s 32.39s 33.74s

So I've our tests are now almost twice as fast :D

@BPScott BPScott changed the title Remove components alias Remove components alias and import components directly Nov 25, 2021
@BPScott BPScott changed the title Remove components alias and import components directly Remove components alias and import components directly in tests Nov 25, 2021
@github-actions
Copy link
Contributor

size-limit report

Path Size
cjs 166.15 KB (0%)
esm 96.71 KB (0%)
esnext 143.41 KB (0%)
css 34.35 KB (0%)

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

I will miss components but this makes sense for the system today.

@BPScott BPScott merged commit 9b367d8 into main Nov 25, 2021
@BPScott BPScott deleted the remove-components-alias branch November 25, 2021 19:38
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Down for this. I usually lean on VS to handle my imports via the quick fix menu

image

The problem now would be that we don't have a meaningful error when someone tries to import from ../components

image

Not a blocker but do you know of a way to fix that?

@BPScott
Copy link
Member Author

BPScott commented Nov 25, 2021

I'm going to open up a follow up PR where I nuke src/components/index.ts. All the component reexports shall happen directly in src/index.ts.

@BPScott
Copy link
Member Author

BPScott commented Nov 26, 2021

There it is ^^ #4739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants