Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

chore: Convert to monorepo #613

Merged
merged 44 commits into from Apr 22, 2022
Merged

chore: Convert to monorepo #613

merged 44 commits into from Apr 22, 2022

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Apr 1, 2022

Converted this repo into a monorepo using turborepo on top of yarn workspaces. Directory structure is split into apps/ and packages/.

  • Moved all iTwinUI-react files into packages/
  • Added a vite playground under apps/ which uses the linked itwinui-react package just for testing components in a more reliable setting compared to storybook.
  • In the future, the documentation website folder will also live under apps/

Most of the changes in this PR are simply additions from turborepo/vite and moving files around (git should show those as "renamed").

The only real "changes" are listed below:

  • Node 16+ / npm 7+ restriction is now set in root package.json.
  • iTwinUI-react build was changed to emit files into cjs/esm folders instead of lib so that it works with symlinks. There is no need to copy package.json into lib anymore.
  • Some commands in github workflows had to be updated to reference itwinui-react instead of running from root.
  • A lot of jest tests which test for tippy.js visibility started failing so I made some changes to get them to pass.
  • Storybook was giving me the following error so I had to add webpack@5 in the root package.json:
  • Eslint+prettier was failing so I upgraded some deps and made changes accordingly.
  • ReactNodeArray is deprecated and was showing an error in Breadcrumbs so I changed it to ReactNode[].

A lot of those issues have nothing to do with turborepo but I think they started appearing because dependencies were updated when lockfile was moved.


Best way to test these changes:

  • Create a fresh clone of this repo and switch to this branch.
  • Run yarn to install everything.
  • Run yarn dev to start vite playground and storybook together.

For running any bespoke commands, you can cd into packages/itwinui-react or just run it from the repo root by prefixing yarn workspace @itwin/itwinui-react.

@mayank99 mayank99 marked this pull request as ready for review April 1, 2022 20:08
@mayank99 mayank99 requested a review from a team as a code owner April 1, 2022 20:08
@mayank99 mayank99 requested review from a team, veekeys and elephantcatdog and removed request for a team April 1, 2022 20:08
Copy link
Member

@veekeys veekeys left a comment

Choose a reason for hiding this comment

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

Storybook and playground did not load for me, so could not really check much.

package.json Outdated
"react",
"ui",
"ux"
"name": "turborepo-basic-shared",
Copy link
Member

Choose a reason for hiding this comment

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

Installing gave me lots of messages of incopatibilities and warning on deps.
We had pretty clean install, so would be nice to keep it like that

image
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there's a lot of new dependencies so it makes sense idk

"classnames": "^2.2.6",
"react-table": "^7.1.0",
"react-transition-group": "^4.1.1"
"build": "turbo run build",
Copy link
Member

Choose a reason for hiding this comment

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

Running yarn dev gave me this output

image

image

image

As a result, storybook didnnt load, even though message said its successfully running

Copy link
Member

Choose a reason for hiding this comment

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

well ok.. idk anymore..
I ran storybook from react package, seemed ok.
Then i ran again yarn dev and now it gave no errors :/ I still have all those prettier changes too, so not sure what happened here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn dev should be fixed now.

Those errors were because the @itwin/itwinui-react package needs to be built before it can be used in the vite playground. So I added ^build dependency and removed the --parallel flag.

Storybook should not be affected, I think it might have been because you had some other storybook running somewhere else.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
packages/iTwinUI-react/stories/core/Header.stories.tsx Outdated Show resolved Hide resolved
packages/iTwinUI-react/tsconfig.json Outdated Show resolved Hide resolved
Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Repo starts, unit test runs and passes.
Main concerns:

  1. Why yarn dev starts both playground and storybook? Is there a reason why it's more useful to look at both when coding/creating than just playground or storybook?
  2. Visual tests are missing (at least I didn't find how to run them). Also creevey as is won't run on Mac M1, we could use this new repo opportunity to switch to other testing suite or fix creevey.

@mayank99
Copy link
Contributor Author

mayank99 commented Apr 14, 2022

  1. Why yarn dev starts both playground and storybook? Is there a reason why it's more useful to look at both when coding/creating than just playground or storybook?

That's the default behavior of turborepo and it also makes sense to me. When I'm working on any component, I have both storybook and test app running.

When we add website to this repo, it will also start with the same command

  1. Visual tests are missing (at least I didn't find how to run them). Also creevey as is won't run on Mac M1, we could use this new repo opportunity to switch to other testing suite or fix creevey.

What visual tests are missing?

image

Good point about using this opportunity. I don't think we will be able to fix creevey but we could switch to cypress or something.

Update: I was playing around with using stories as cypress tests and got it running after a couple hours. But it's a lot of changes, so maybe it should be in a separate PR?

@mayank99
Copy link
Contributor Author

mayank99 commented Apr 18, 2022

I've fixed linting/formatting issues by making the following changes:

  • As per turborepo's recommendation, I moved all configs into a shared configs package and added that package as a dev dependency in other packages.
  • As per prettier recommendation, I'm now using only eslint-config-prettier and have removed eslint-plugin-prettier. This seems to fix the conflicts I was getting between prettier and eslint.
  • As per eslint recommendation, I've added quotes around paths, which seemed to fixed the problem of eslint doing nothing when running yarn lint.
  • As per lint-staged recommendation, I've added lint-staged command to root package.json to forward it to individual packages. Tested and it's working fine after I added --max-warnings=0 to the eslint command.

'plugin:@typescript-eslint/recommended', // Uses the recommended rules from @typescript-eslint/eslint-plugin
'plugin:react-hooks/recommended',
'plugin:storybook/recommended',
'prettier',
Copy link
Member

Choose a reason for hiding this comment

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

we had this before too plugin:prettier/recommended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

],
rules: {
// Place to specify ESLint rules. Can be used to overwrite rules specified from the extended configs
'react/prop-types': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

and here this "prettier/prettier": [ 1 ],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@@ -0,0 +1,4 @@
{
"extends": "configs/tsconfig.base.json",
Copy link
Member

Choose a reason for hiding this comment

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

is this path correct? it shows error to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is correct. configs is the name of the package where I've moved this file.

Screen.Recording.2022-04-21.at.9.59.13.AM.mov

@veekeys
Copy link
Member

veekeys commented Apr 21, 2022

Linter seems to work fine. I have tried messing up code and checking if on save fixes minor things.
Also checked, if yarn lint will catch warning (like any type), which worked fine too.
Really appreciate the links for recommendations, cause otherwise, it would be hard to understand the structure.
Just weird, that tsconfig error was not notified anywhere.. wonder if there are some more like that left

Copy link
Member

@veekeys veekeys left a comment

Choose a reason for hiding this comment

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

Good job!
Welcome, Turborepo!

Copy link
Contributor

@bentleyvk bentleyvk left a comment

Choose a reason for hiding this comment

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

Monorepo it is

@mayank99 mayank99 merged commit f849155 into main Apr 22, 2022
@mayank99 mayank99 deleted the mayank/monorepo branch April 22, 2022 13:05
mayank99 added a commit to iTwin/iTwinUI that referenced this pull request Dec 21, 2022
Converted this repo into a monorepo using turborepo on top of yarn workspaces. Directory structure is split into apps/ and packages/.

- Moved all iTwinUI-react and config files into packages/
- Added a vite playground under apps/ which uses the linked itwinui-react package just for testing components in a more reliable setting compared to storybook.
- In the future, the documentation website folder will also live under apps/

lockfile was recreated so there were a lot of random changes I had to make for new deps, mostly in tests.
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.

None yet

4 participants