-
Notifications
You must be signed in to change notification settings - Fork 3k
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
chore: Move to monorepo ❗ ❗ ❗ #7342
Conversation
Reorganized everything. Removed broken tests from docs app so we can run tests in CI for that again. Some tests are better than no tests. Updates the GitHub actions, some of the README stuff, and build scripts
Note to @jakovljevic-mladen and @niklas-wortmann: To build or run the docs app now you'll want to use yarn workspaces: After you initially pull this down, you'll want to delete your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of issues found during code review.
I will have to test this locally which I will do later this week (hopefully).
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
…dex.js" This reverts commit c734d1e.
@jakovljevic-mladen I had to revert both of your suggestions for resolving directory/file locations that I committed above. They broke the docs app build. If you think those are incorrect, I suggest we fix it in a follow up. Otherwise, I think this is good to go. |
I'm going to go ahead and merge this so we can move on. @jakovljevic-mladen you can double-check the docs app to make sure everything is fine, but I ran it locally and ran through a few user flows just to see if it was okay, and everything checked out fine. |
@benlesh, I guess I was wrong about |
packages/rxjs
: The new home for the mainrxjs
package.apps/rxjs.dev
: The new home for the docs apppackages/*
: will be where other future packages live (i.e.@rxjs/observable
)apps/*
directory at some point.lodash
(and@types/lodash
): This was done to fix problems with the TS@latest tests we run in CI. This is because the newer version of TypeScript fixed the types for WeakMap, and the older version of@types/lodash
had them redefined differently. This is a small change../
to./packages/rxjs/
in some cases.rxjs
package when we publish.npm install
for workspaces will hoist all packages to the top level. When that's done, both@types/mocha
(from rxjs) and@types/jasmine
(from the docs app) are hoisted to the rootnode_modules
folder. When that happens, TypeScript chokes on build because of duplicate type definitions for things likedescribe
,xit
, etc. Yarn features a nicenohoist
setting, and we'll have to stick with Yarn until npm decides how they're going to fix it