Move from local icons to polaris-icons

[amrocha]

WHY are these changes introduced?

There's an ongoing project to improve icon usage at Shopify (https://unicorn.shopify.io/projects/4785).

As part of this project we're making a new central repository to store our icons, and deploying a package from said repo containing all of our Icons.

This PR deletes all of the local icons, and switches to use polaris-icons instead. This change should be imperceptible to the user, and is only an architecture change.

WHAT is this pull request doing?

Deleting local icons, replacing them with icons from the polaris-icons package. This is purely a plumbing change, and should have no effect on the end-user's experience.

How to 🎩

Make sure bundle sizes don't change in consuming projects.

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated UNRELEASED.md

[amrocha]

@BPScott @dfmcphee @kaelig @tmlayton @dleroux Hello friends, this PR is the first step in the rollout of the icons project.

We originally talked about merging this into a beta branch, but looking at the scope of the changes, and considering that this change is purely architectural and shouldn't affect the end-user, I thought there might be a case for just merging this into master. I'm pinging all of you to know what your opinion is on this, and so that people across the team are aware that this is happening.

Do you think we can merge this change into master directly? Any reservations, or feedback on the approach taken here?

[BPScott]

Anything that matches icons\/ would also match anything that matches polaris-icons\/ so I don't think this second regex is needed.

If we wanted to be stricter and match an exact folder name we could still do it in single regexp I think. Something like /\/(icons|polaris-icons)\// would match both blah/icons/blah and blah/polais-icons/blah.

[BPScott]

Fantastic work! Keeping src/icons/index lets us maintain backwards compatibility and suggests to people that if they need a polaris icon they should grab it via @shopify/polaris/icons rather than referencing the polaris-icons package directly. Which from our chat yesterday is the direction we want to head in.

I am a little wary of having a stable release of polaris depend upon a beta release of polaris-icons. I don't think we want to give that impression of beta unstableness to 3rd parties that consume polaris - even though I know this isn't going to result in any breaking changes or churn for consumers of polaris.

I'm super excited by how close this is but I think it is worth getting those last bits of polish in the polaris-icons repo around using a monorepo structure and investigating making an esnext build, then cutting a v1.0.0 before we merge this into master

[dleroux]

Really happy to see this taking shape!

[amrocha]

I am a little wary of having a stable release of polaris depend upon a beta release of polaris-icons. I don't think we want to give that impression of beta unstableness to 3rd parties that consume polaris - even though I know this isn't going to result in any breaking changes or churn for consumers of polaris.

This is a very good point, we wouldn't want to give our users the impression of fragility.

I'm super excited by how close this is but I think it is worth getting those last bits of polish in the polaris-icons repo around using a monorepo structure and investigating making an esnext build, then cutting a v1.0.0 before we merge this into master

I'm already working on the monorepo, but that's not the reason I decided to make the package a beta. Like we talked about yesterday, there might be a better way of structuring exports so that tree-shaking is easier. If it did come to that, I would have to make breaking changes to the package. To avoid version numbers quickly growing I decided that making it a beta until we tried this out in production and were confident of it was the right choice.

Do you think this is unnecessary? Maybe it's fine if we increase the major version numbers at first.

[BPScott]

Yep the changing exports thing is what I was more concerned about from a breaking-changes perspective. I totally agree with using beta versions to give us a chance to make those initial breaking changes without a spate of major version bumps.

(My personal approach for that tends to be using 0.X.Y versions till I'm confident I've got a stable external API but using 1.0.0-beta-X has exactly the same effect)

[kaelig]

For context, the first time @shopify/polaris-tokens package was added as a dependency, it was still in an unstable stage. At the time it used a 0.x.x version.

As 0.x.x semver ranges can be confusing (e.g. ~0.2.1 and ^0.2.1 resolve similarly…), I suggested we use the 1.0.0-beta.x versioning scheme in @shopify/polaris-icons.

Edit: @shopify/polaris has shipped with dependencies in beta in the past (e.g. @shopify/react-utilities). It hasn't eroded trust, as far as we can tell.

[amrocha]

I'm super excited by how close this is but I think it is worth getting those last bits of polish in the polaris-icons repo around using a monorepo structure and investigating making an esnext build, then cutting a v1.0.0 before we merge this into master

polaris-icons is now a monorepo!

As for the esnext build, I also looked into that.

Tree-shaking only works with ESM modules. CJS is incompatible with ESM, so tree-shaking won't work out of the box with polaris-icons. This might seem bad at first, but I actually think it's fine. What this means to us is that if we want to benefit from tree-shaking then we can't import icons directly from polaris-icons, and have to use an intermediary file that imports the entire bundle, and exports individual icons. This is exactly the approach in this PR, and the way we want people to use polaris-icons!!

Because of this, I think it's ok to expose a single CJS script. By enforcing this structure, devs also get the benefits of typing and integration with IDEs, which will provide a better experience overall.

[amrocha]

All of the issues brought up have been addressed, and we're ready to move forward with this PR.

I will be merging this PR around 3PM EST. If you have any questions or concerns please bring them up before then.

[lemonmade]

@amrocha I am not sure I understand how the existing approach actually does tree shaking. When you do an import of an object (a default import or namespace import; in this case it is the default import from @shopify/polaris-icons, no part of that object can ever be tree shaken. It does not matter if a consumer only pulls in one icon, the import of the icon object (and all the icons with it) will end up in the main bundle. Attempting to build using the ESNext build of Polaris (importing only one icon-using component) should illustrate this problem.

For tree shaking to work, you must have an ESNext (that is: preserves import/ export statements) version of Polaris icons, and you must not import them in Polaris in any way that brings them in as a single object with properties for every icon.

[amrocha]

@lemonmade Thanks for pointing that out. I did some more extensive testing locally and it seems like you're right, the entire package gets imported into the bundle.

I was under the impression that the project would be able to tree shake icons that weren't used if we had a setup like the icons/index.ts file, that imports an object from the library and exports individual icons. It seems like the object with all the icons still gets imported into the bundle regardless though.

I will look into exporting individual icons from polaris-icons, which would enable consumers to tree shake the package.

That being said, currently both the ESM and CJS builds of polaris-react import every single icon into the bundle, so this is not a regression, it just maintains the status quo. So, In order to start getting feedback on other parts of the polaris-icons project I want to go ahead and merge this PR. Do you think that's ok, or do you think this will cause a major regression in web?

[amrocha]

Talked to Chris and we agree on shipping this as is since it's not a regression. There's a release going on right now though so I'll give it a day in case something goes wrong and we need to rollback.

New plan is to merge tomorrow around noon

[kaelig]

Worth mentioning this in the changelog - in case anything goes wrong it'll be easier for people to track the possible source back to this PR.