Skip to content

Migrate to yarn!#591

Merged
shweaver-MSFT merged 9 commits intomainfrom
shweaver/yarn
Aug 27, 2020
Merged

Migrate to yarn!#591
shweaver-MSFT merged 9 commits intomainfrom
shweaver/yarn

Conversation

@shweaver-MSFT
Copy link
Copy Markdown
Contributor

@shweaver-MSFT shweaver-MSFT commented Aug 25, 2020

Closes #592

PR Type

  • Build or CI related changes

Description of the changes

In this PR I've integrated the popular npm alternative, Yarn!

The setup is super simple:

# If you haven't already, install yarn globally
npm i -g yarn

# Install dependencies in node_modules
yarn

# Build the project
yarn build

# Watch and serve
yarn start

Use yarn commands

The notable difference for us and our contributors, is that commands now should be run with yarn instead of npm run (yarn is nice and doesn't require an extra run command)

To install dependencies, run yarn instead of npm i

When adding new dependencies to the project, you can use yard add package-name instead of npm i package-name

If adding a new dependency package to the root, use yarn add -W package-name
The -W is important; It tells yarn to apply it to the root workspace.

Basically run yarn anywhere you would normally run npm.

Exception
When packaging the project, use yarn run pack instead of yarn pack. Running without will attempt to package the root, which doesn't make much sense in a monorepo.

PR checklist

  • Project builds (npm run build) and changes have been tested in supported browsers (including IE11)
  • All public classes and methods have been documented
  • Added appropriate documentation [target branch mgt/next for new features]. Docs PR:
  • License header has been added to all new source files (npm run setLicense)
  • Contains NO breaking changes

Other information

@ghost
Copy link
Copy Markdown

ghost commented Aug 25, 2020

Thank you for creating a Pull Request @shweaver-MSFT.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@nmetulev
Copy link
Copy Markdown
Contributor

If adding a new dependency package to the root, use yarn add -W package-name
The -W is important; It tells yarn to apply it to the root workspace.

Does this mean that when we add a dependency package to, let's say mgt-element, instead of doing npm install package, we should do the above? Do you mind sharing a quick example of how this would be used? And should this be part of the contributing file?

Copy link
Copy Markdown
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

🍾 🍾 🍾

Seems to work great. I still need to test properly, but starting with few initial comments and questions

@shweaver-MSFT
Copy link
Copy Markdown
Contributor Author

If adding a new dependency package to the root, use yarn add -W package-name
The -W is important; It tells yarn to apply it to the root workspace.

Does this mean that when we add a dependency package to, let's say mgt-element, instead of doing npm install package, we should do the above? Do you mind sharing a quick example of how this would be used? And should this be part of the contributing file?

Sorry to confuse, the -W flag is really only applicable when adding packages to the root. We do that for things like storybook, lerna, assorted devDependecies for various package scripts. But we don't need/want to declare the dependencies for any of the actual packages in the root.

For new package dependencies, those should be installed locally to their package:

# Literally go to the directory
cd ./package/mgt-whatever

# Install the dependency
yarn add some-package-name

@shweaver-MSFT shweaver-MSFT requested a review from nmetulev August 26, 2020 23:35
@shweaver-MSFT shweaver-MSFT self-assigned this Aug 26, 2020
Copy link
Copy Markdown
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

Most of the samples were broken - you've updated the imports but doesn't look like you've tested. Since you moved some samples to use the local es6 modules, you should have also added type="module" to the scripts and used imports instead of mgt.. I had to update the samples to test them anyway, so I've committed it directly here. See commit for the changes I've made.

Otherwise, with minor comments, looks good and can be merged

Comment on lines +9 to +10
"build:mgt": "cd ./packages/mgt && npm run build",
"build:mgt-element": "cd ./packages/mgt-element && npm run build",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These no longer seem necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kind of like them. Makes it easier to build a specific project without having to change directories. It's an optimization imo :)

@shweaver-MSFT shweaver-MSFT merged commit a50245e into main Aug 27, 2020
@shweaver-MSFT shweaver-MSFT deleted the shweaver/yarn branch August 27, 2020 16:23
@shweaver-MSFT
Copy link
Copy Markdown
Contributor Author

@nmetulev thanks for you help on the samples! Excited to see this merged in 🎉😁

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.

Integrate yarn to simplify dependency structure in the monorepo

2 participants