Skip to content

Initialize a Rush workspace for client SDKs #2047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 29, 2019
Merged

Initialize a Rush workspace for client SDKs #2047

merged 12 commits into from
Apr 29, 2019

Conversation

bsiegel
Copy link
Member

@bsiegel bsiegel commented Apr 5, 2019

This PR creates a new Rush workspace, currently only containing the client SDKs. It includes several dependency version updates to align them across projects - this way, Rush can ensure that dependency versions for all of our packages remain in sync going forward.

Rush is configured to use pnpm as the package manager which provides several additional benefits. However Rush is not 100% compatible with the current version of pnpm (v3) so we should hold off merging this until that compatibility is added, or roll back to v2 of pnpm in rush.json

@weshaggard
Copy link
Member

cc @Azure/azure-sdk-eng

@bsiegel bsiegel force-pushed the rush_init branch 2 times, most recently from 397392f to 8c275db Compare April 11, 2019 23:22
@bsiegel bsiegel force-pushed the rush_init branch 11 times, most recently from 44b2e5a to faa5885 Compare April 18, 2019 23:05
@bterlson
Copy link
Member

Some notes:

  • main key of rollup-plugin-node-resolve is deprecated. Use mainFields instead.
  • Seems like we have multiple versions of TypeScript. Probably not a major issue for now.
  • rush test rebuilds the world after a rush build. Is this expected?
  • rush add should really allow me to add local, unpublished packages (see: [rush] "rush add" should be able to add unpublished locally-linked projects microsoft/rushstack#991)
  • Instructions for taking an unpublished dependency seems important. Process is, basically, manually update your package.json with the dependency and run rush update.
  • VSCode seems to stomp on some things, resulting in permissions errors when doing e.g. rush update. I suspect it's automatic type acquisition. I think I've gotten to a good place by enabling "typescript.disableAutomaticTypeAcquisition" in my workspace, might be worth adding that suggestion to the readme.

@bterlson
Copy link
Member

Played around with this (and built on it with #2373) and I like it lots. Happy to see this merged ASAP so we can start more refactorings like #2373.

@bsiegel
Copy link
Member Author

bsiegel commented Apr 22, 2019

  • main key of rollup-plugin-node-resolve is deprecated. Use mainFields instead.

Fixed.

  • Seems like we have multiple versions of TypeScript. Probably not a major issue for now.

This was always true. API Extractor v6 requires an old version. If we upgrade to v7 it will resolve this issue.

  • rush test rebuilds the world after a rush build. Is this expected?

No, it's because we have many packages where the "test" script runs "build". We should clean this up in a followup PR.

😿 but for what it's worth you can edit the package.json manually and do rush update in this situation.

  • Instructions for taking an unpublished dependency seems important. Process is, basically, manually update your package.json with the dependency and run rush update.

Good catch! Fixed.

  • VSCode seems to stomp on some things, resulting in permissions errors when doing e.g. rush update. I suspect it's automatic type acquisition. I think I've gotten to a good place by enabling "typescript.disableAutomaticTypeAcquisition" in my workspace, might be worth adding that suggestion to the readme.

Added to the README. What are your feelings on checking in the .vscode/settings.json file with this change? I can see other settings being appropriate to commit as well, to help enforce some things repo-wide. If we want to make sure people don't keep getting diffs by making settings changes to their local workspace, we can keep the file .gitignored and force-add it to the working tree.

@bterlson
Copy link
Member

I have a vague worry that there are other things in vscode doing npm install for you, but talking with @ramya-rao-a this may be the only place. I like disabling it in settings.json. I also kinda like the hack you mention to allow updating of local settings! It lets me configure my extensions I love that nobody else has 😊 But, I'll leave that up to you, I'm ok dealing with untracked changes as well.

@bsiegel bsiegel force-pushed the rush_init branch 2 times, most recently from 14f99ea to c7a893d Compare April 22, 2019 23:40
Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

A few general questions:

  1. Should the Azure Pipelines YAML be updated to use the Rush commands for building, testing, etc?
  2. How does build and test performance compare between the current "npm-run-all" solution and Rush? What, if anything, does Rush improve with respect to developer experience, either inner loop (incremental compilation) or outer loop (full compilation)?

@bterlson
Copy link
Member

FWIW, my experience is full builds are much faster as it seems to skip things that are already built.

@mikeharder
Copy link
Member

FWIW, my experience is full builds are much faster as it seems to skip things that are already built.

I will run a few before/after perf tests.

@bsiegel
Copy link
Member Author

bsiegel commented Apr 23, 2019

  1. Should the Azure Pipelines YAML be updated to use the Rush commands for building, testing, etc?

Yep, I am planning to do that in a followup PR.

@bsiegel bsiegel force-pushed the rush_init branch 3 times, most recently from 31c4e0b to 8fdbcee Compare April 25, 2019 00:01
@bsiegel bsiegel force-pushed the rush_init branch 2 times, most recently from eb7d3b3 to 8aeffa1 Compare April 26, 2019 22:30
@bsiegel bsiegel merged commit 0087296 into master Apr 29, 2019
@bsiegel bsiegel deleted the rush_init branch April 29, 2019 18:57
@@ -0,0 +1,278 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I accidentally posted this to another PR, not sure what happened, and I know this is already merged but I'm still curious about my question below.

Is there any hard rules about these files going into a folder called common? I ask mostly as for some of the other languages we have been putting similar engineering system type files under an "eng" directory.

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.

4 participants