[Tests] - updating Vitest breaks most tests

[Powerplex]

Dependabot opened a PR to update Vitest, and it breaks many tests. https://github.com/adevinta/spark/pull/1458

I checked the vitest update (it seems they don't have a proper changelog) and it starts breaking our test from version 0.34.0 onward.

It seems we might need to also update @vitejs/plugin-react at the same time. I also noticed we reference Vitest twice (dependencies and devDependencies), which is strange.

Here is the type of error that appears:

[acd02]

So, after investigating this issue, here’s the rundown:

1. First, I updated the vitest package to version 0.34.6. a. When running the tests, I encountered an error message in the CLI that directed me to the @vitejs/plugin-react repository.
2. So, I updated the @vitejs/plugin-react to version 4.1.0. Previously, we were using the last major version, 3.1.0. a. When running the tests, I encountered the error React is not defined. b. I checked the release notes for version 4 of the @vitejs/plugin-react. c. In the release notes, I found the following message 👇: d. “The support for React auto import when using classic runtime is removed. This was prone to errors and added complexity for no good reason given the very wide support of automatic runtime nowadays. This migration path should be as simple as removing the runtime option from the config.” e. So, I removed the runtime option from the config/component.ts file. f. I then tried to rebuild the packages without this option, and it threw an error every time we had an import statement importing stuff from a dist folder (such as @spark-ui/icons/dist/icons/**) in our packages.

---

Based on these findings, I see three options:

First option: (revert ⏮️)

We can revert to what we had a couple of months ago. Instead of importing Icons from the dist folders inside our components, we can import them directly from @spark-ui/icons. However, this will break tree shaking 🥲, which means that if someone imports a single Spark component using a single icon, they will also import the code for all 437 icons. I don’t think we want that

Second option: (tweak 🔧)

We would have to update the config/component.ts file as follows:

const deps = Object.keys(pkg.dependencies || {})
const devDeps = Object.keys(pkg.devDependencies || {})
const omitDeps = Object.keys(pkg.omit || {}) // 👈 new

...

rollupOptions: {
  external: [...deps, ...devDeps, ...omitDeps],
}

Then, we would also need to modify the package.json files of our components and add a new field (something like omit, skip, or ignore 🤷) to declare the icons used within a component. This will ensure that they are properly excluded during the build process (and as a result, the tests won’t break anymore). For example, if we have a Foo component that uses the ArrowVerticalLeft and ArrowVerticalRight icons, we would still import the icons using the dist folder as we currently do. But to prevent build failures, we would need to modify the package.json for this component as follows:

// package.json

"name": "@spark-ui/foo",
"dependencies": { ... },
"omit": {
  "@spark-ui/icons/dist/icons/ArrowVerticalLeft": "",
  "@spark-ui/icons/dist/icons/ArrowVerticalRight": "",
}

This approach is a bit of a hack, but it works. Are we okay with this? 🤷

Third option: (clean 🧹)

Perhaps it’s time to reconsider our icon set. Currently, we have almost ~450 icons 🤢, and this number will continue to grow over time. However, 99% of these icons are not even used by our internal components. They exist within our repository but are only consumed by Polaris in the end. It may be appropriate to keep only the icons that we actually use within our components (a couple of dozen at most) and have a dedicated repository for all the other icons

[turolopezsanabria]

~450 icons is CRAZZYYY, we must do a cleanup for sure

[acd02]

We had a meeting called remove icons from Spark repository on October 18th, we discussed the possibility of separating the Spark core icons (icons used internally by Spark components) from the rest (which are Polaris icons).

Ultimately, all stacks (web, iOS, android) agreed that we should indeed separate them into two packages/repos:

• one for core Spark icons
• and a dedicated one for the rest (which are icons used by Polaris )

However, to make this happen, we will need to update the current pipelines, also this will involve updates on Polaris (such as updating imports and stuff).

Therefore, we will need to create tickets to address this during the next sprints

[!Important]
In the meantime, this ticket will be considered as blocked.

[thomas-leguellec]

We had too many icons, should be ok. Need to check if it's ok with the new repo.

[Powerplex]

@acd02 @andresin87 @soykje this ticket has been here since September. What do we do with it ?

[acd02]

👋 @Powerplex , To go forward with this ticket, we should first decide which icons we will keep/use in our repo