-
Notifications
You must be signed in to change notification settings - Fork 900
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
Monorepo Refactor #171
Monorepo Refactor #171
Conversation
WIP: testing Refactor folder structure to use lerna WIP: more testing feat(util): add deferred class Publish - @firebase/[email protected] - @firebase/[email protected] WIP: asdoifj WIP: build artifact WIP: asdf Cleaning up from .gitignore Add prepublish script Isolate base configs Making top level scripts parallel Adding storage Add messaging Adding database code TODO: Fix all of the broken issues and import the rest of the utils Adding type info add database Adding firebase package Generating ES Module builds Attaching firebase.js to the global scope Adding lint-staged to the build Removing commitizen dependency Updating metafiles Working on devx Add react-native package Move packages/firebase -> packages/core Fix issues with package.json Bump core version testing a thing Add more entries to the .npmignore
@mikelehen I see what you're saying.... I hadn't thought of that necessarily. I typically work w/ the whole thing open but it's because I work w/ more cross-cutting changes. If it's alright let's come back to that. I think it's worth thinking about but I'd like to see VSCode have a little better support for having nested |
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.
The storage changes LGTM. README instructions for build + test worked on first try.
Rubber stamping this. |
Can you elaborate a little bit on why we had to move to 'yarn' and Node 8? I feel like the entry barrier for contributors is already pretty high and it would be nice to keep it low. The GCloud SDKs for example run and are developed with Node 4 + npm for example. But on the other hand test setup is really cool & simple. Thanks for this. I do get the following warning though when I run the database tests: If this is something we can fix, then maybe we should! :) |
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.
LGTM. Please address my concerns at your discretion.
@schmidt-sebastian I have addressed the license issues (actually found a couple others per that comment) and so we should be good there.
Node 8 was just to enable easier development of tooling and things within the SDK. Since we are compiling our source for use everywhere, it doesn't seem unreasonable to empower the development cycle w/ the latest syntax. |
Overview
This PR refactors the SDK from a single repository, with 1 NPM module (i.e.
firebase
), to a monorepo format with many NPM modules. Most of this PR is structural but there have been some changes made to cross-module dependencies due to the newly expose modules.As a result of this PR we will be publishing the following packages under the
firebase
scope on NPM:@firebase/app
@firebase/auth
@firebase/database
@firebase/firestore
@firebase/messaging
@firebase/polyfill
@firebase/storage
@firebase/util
Each of these packages will be independently versioned which will lay the groundwork for more concrete cross module communication going forward.
Component Changes
This is a huge change, so to help with the review, I have broken down the review by component below. @gauntface, @mikelehen, @sphippen, @bojeil-google, @schmidt-sebastian take a look at the sections below. Feel free to review more than just your component, though I don't necessarily expect that.
Auth
Click to see details
All of this work can be found here:
/packages/auth
src/auth.build.js
file topackages/auth/src/auth.js
package.json
,.npmignore
, etc)packages/auth/gulpfile.js
)@firebase/auth
and expose it viafirebase/auth
@firebase/auth
into the mainfirebase.js
binary`src/auth.js
file to the.prettierignore
to ensure that we don't accidentally format the minified source.Database
Click to see details
All of this work can be found here:
/packages/database
src/database
directory topackages/database/src
src/database.ts
file topackages/database/index.ts
tests/database
directory topackages/database/test
packages/database/src
to properly reference@firebase/app
and@firebase/util
as needed.packages/database/index.node.ts
that imports the Node specific bits.NOTE: You can view the
database
specific changes in that commit by searching forpackages/database
in the commit above. You will see all of the file renames and required changes. Give it a minute to load the whole commit.Firestore
Click to see details
All of this work can be found here:
/packages/firestore
There are several commits here that I intentionally didn't squash to help w/ the review of this.
@firebase/app
module)@firebase/polyfill
firestore/index.ts
to properly reference the implementation@firebase/app
firestore
module typings and fixed typings references to @firebase/appIn addition
packages/webchannel-wrapper
has been imported from https://github.com/firebaseprivate/webchannel-wrapperMessaging
Click to see details
All of this work can be found here:
/packages/messaging
src/messaging
directory topackages/messaging/src
src/messaging.ts
file topackages/messaging/index.ts
tests/messaging
directory topackages/messaging/test
packages/messaging/src
to properly reference@firebase/app
and@firebase/util
as needed.integration/messaging
to properly point to the newly createdfirebase
libNOTE: You can view the
messaging
specific changes in that commit by searching forpackages/messaging
in the commit above. You will see all of the file renames and required changes. Give it a minute to load the whole commit.Storage
Click to see details
All of this work can be found here:
/packages/storage
src/storage
directory topackages/storage/src
src/storage.ts
file topackages/storage/index.ts
tests/storage
directory topackages/storage/test
packages/storage/src
to properly reference@firebase/app
and@firebase/util
as needed.NOTE: You can view the
storage
specific changes in that commit by searching forpackages/storage
in the commit above. You will see all of the file renames and required changes. Give it a minute to load the whole commit.Other Changes
git cz
, or formatted git messages)prepush
action instead ofprecommit
prepush
validation (thanks @wilhuff for this idea)test:setup
NPM script that will do most of the work of setting up a Firebase test project for the user.Workflow Changes
For development, you can simply run
yarn dev
at the root of the package. This will spin up watchers on all the source files and will recompile as you develop. There is a development server available athttp://localhost:8080
that will expose thefirebase
namespace so you can play w/ your changes.NOTE: I was considering also adding test watchers, but that got noisy, if we want that I can add it w/o too much trouble
yarn
instead ofnpm
, this new workflow requiresyarn workspaces
. I will be updating the CONTRIBUTING.md to talk about this, but I wanted to get this out. I'll update this once it is ready. In short, please useyarn
>1.0.0 and you're good to go.tl;dr
Ping me w/ questions, as you have them! I'd be happy to explain anything/everything I did.