Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Add images-no-direct-imports rule #219

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Feb 9, 2019

The Polaris team has been doing some research into how icons (and kinda images in general) get used.

One thing that was identified was that we strongly tend towards putting images in a dedicated folder within a component (overwhelmingly either images, icons or illustrations) then create an index file for that folder. Then within the component you import from that index file.

Once https://github.com/Shopify/web/pull/10568 is merged then the stats will be: 494 svg files are imported into an index, 36 are imported elsewhere.

images-no-direct-imports is a new rule to enforce the convention of creating an index file by disallowing imports of image files into any file that is not the index file in the same directory as the svg resides.


Open questions:

  • What's the preferred convention for namespaced rules? A prefix to the filename (e.g. react-type-state) or create a folder (e.g. typescript/prefer-singular-enums)
  • Whats the preferred naming between 'no-BAD-THING' or 'prefer-GOOD-THING' - should this be no-direct-imports or prefer-folder-imports (or something to that effect)
  • How should we enable this rule: Create a new config called "images"? Shoehorn it into polaris config?

Copy link
Contributor

@cartogram cartogram left a comment

Choose a reason for hiding this comment

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

Code makes sense to me, just made some suggests for the way you describe this rule.

docs/rules/images-no-direct-imports.md Outdated Show resolved Hide resolved
docs/rules/images-no-direct-imports.md Outdated Show resolved Hide resolved
docs/rules/images-no-direct-imports.md Outdated Show resolved Hide resolved
docs/rules/images-no-direct-imports.md Outdated Show resolved Hide resolved
lib/rules/images-no-direct-imports.js Outdated Show resolved Hide resolved
lib/rules/images-no-direct-imports.js Outdated Show resolved Hide resolved
@cartogram
Copy link
Contributor

cartogram commented Feb 9, 2019

  • What's the preferred convention for namespaced rules? A prefix to the filename (e.g. react-type-state) or create a folder (e.g. typescript/prefer-singular-enums)

In this case, I don't think you need a folder prefix. Do we imagine a lot of images related rules? Probably not.

  • Whats the preferred naming between 'no-BAD-THING' or 'prefer-GOOD-THING' - should this be no-direct-imports or prefer-folder-imports (or something to that effect)

Wish I had a good answer and I imagine we are pretty inconsistent. I think there are official standards out there somewhere but I couldn't find them unfortunately. In the past, I've just chosen the one that sounds right to me 🤷‍♂️

In this case I would go with prefer, but images-prefer-no-direct-imports seems long-winded and I like what you have.

  • How should we enable this rule: Create a new config called "images"? Shoehorn it into polaris config?

I don't think an images config is worthwhile. I think just keep it amongst the other generic ones, this has nothing to do with polaris specifically. If anything, we have a number of file-location-y/import (strict-component-boundaries, no-ancestry-direct-import) rules that would be a more appropriate config-grouping for this.

Also, remember to add this to the Readme, Changelog, etc...

@BPScott
Copy link
Member Author

BPScott commented Feb 11, 2019

In this case, I don't think you need a folder prefix. Do we imagine a lot of images related rules? Probably not.

There might be one or two more down the line but that's it. No folder it is :)

In this case I would go with prefer, but images-prefer-no-direct-imports seems long-winded and I like what you have.

I was thinking that if we do "prefer" then what comes after becomes a positive statement - images-prefer-folder-import or images-prefer-folder-index-import. but neither of those grab me as being WAY better than images-no-direct-imports.

don't think an images config is worthwhile. I think just keep it amongst the other generic ones, this has nothing to do with polaris specifically

Cool. I'll toss it into the same config as strict-component-boundaries and no-ancestry-direct-import.

Thanks for being a sounding board for my overthinking :)

@BPScott BPScott merged commit fd2e350 into master Feb 13, 2019
@BPScott BPScott deleted the images-no-direct-imports branch February 13, 2019 19:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants