Skip to content
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

Merged
merged 139 commits into from Oct 7, 2017
Merged

Monorepo Refactor #171

merged 139 commits into from Oct 7, 2017

Conversation

jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Sep 27, 2017

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

  • 74246ae - This is the commit where this work was completed. I did the following:
    • Migrated the src/auth.build.js file to packages/auth/src/auth.js
    • Added base package files (i.e. package.json, .npmignore, etc)
    • Created a minimal build script for the auth file (see packages/auth/gulpfile.js)
    • Updated the firebase package dependency to include @firebase/auth and expose it via firebase/auth
    • Bundle @firebase/auth into the main firebase.js binary`
    • Added the 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

  • 93aef0a - The individual commits for this file were squashed to reduce merge difficulties going forward. Database, storage, messaging, and app, because of the unified build process, all followed basically the same migration path:
    • Migrated the src/database directory to packages/database/src
    • Migrated the src/database.ts file to packages/database/index.ts
    • Migrated the tests/database directory to packages/database/test
    • Refactor the paths in packages/database/src to properly reference @firebase/app and @firebase/util as needed.
    • Expose a packages/database/index.node.ts that imports the Node specific bits.

NOTE: You can view the database specific changes in that commit by searching for packages/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.

  • 85e3b91 - Introduction of the firestore boilerplate
  • 2430768 - Moving of the firestore src/test files
  • 7b4717f - Remove unneeded internal typings (you can depend directly on these types as exported by the @firebase/app module)
  • edd052b - Remove promise reference as this is done automatically via @firebase/polyfill
  • e7adf9b - Refactored firestore/index.ts to properly reference the implementation
  • d45f0b8 - Fixed test paths
  • bc038c1 - Fixed reference to the @firebase/app
  • 63de02d - Reintroduced the ambient firestore module typings and fixed typings references to @firebase/app
  • 8302c7f - Got FIrestore node tests working
  • e4916e5 - Added firestore to the top level build and dev scripts
  • bdfbeb0 - Fixed Firestore browser tests
  • b519a5e - Fixed firestore VSCode scripts

In addition packages/webchannel-wrapper has been imported from https://github.com/firebaseprivate/webchannel-wrapper

Messaging

Click to see details

All of this work can be found here: /packages/messaging

  • 93aef0a - The individual commits for this file were squashed to reduce merge difficulties going forward. Database, storage, messaging, and app, because of the unified build process, all followed basically the same migration path:
    • Migrated the src/messaging directory to packages/messaging/src
    • Migrated the src/messaging.ts file to packages/messaging/index.ts
    • Migrated the tests/messaging directory to packages/messaging/test
    • Refactor the paths in packages/messaging/src to properly reference @firebase/app and @firebase/util as needed.
    • Fix integration/messaging to properly point to the newly created firebase lib

NOTE: You can view the messaging specific changes in that commit by searching for packages/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

  • 93aef0a - The individual commits for this file were squashed to reduce merge difficulties going forward. Database, storage, messaging, and app, because of the unified build process, all followed basically the same migration path:
    • Migrated the src/storage directory to packages/storage/src
    • Migrated the src/storage.ts file to packages/storage/index.ts
    • Migrated the tests/storage directory to packages/storage/test
    • Refactor the paths in 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 for packages/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

  • Removed the commit message validation (no more git cz, or formatted git messages)
  • Refactored the prettier code styling to be a prepush action instead of precommit
  • Added a license header validation step to the prepush validation (thanks @wilhuff for this idea)
  • Improved error messaging around testing/development
  • Added a 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 at http://localhost:8080 that will expose the firebase 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 of npm, this new workflow requires yarn 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 use yarn >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.

WIP: testing

Refactor folder structure to use lerna

WIP: more testing

feat(util): add deferred class

Publish

 - @firebase/app@0.1.1
 - @firebase/util@0.2.0

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
@jshcrowthe jshcrowthe changed the title [DO NOT MERGE] - Yarn Workspaces + Lerna Refactoring Effort [DO NOT MERGE] - Yarn Workspaces + Lerna Refactoring (a.k.a Monorepo Refactor) Sep 27, 2017
@jshcrowthe
Copy link
Contributor Author

@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 .vscode dirs so people who work w/ all context and people developing just in the package can both have a solid experience.

Copy link
Contributor

@sphippen sphippen left a 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.

@bojeil-google
Copy link
Contributor

Rubber stamping this.

@schmidt-sebastian
Copy link
Contributor

schmidt-sebastian commented Oct 7, 2017

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:
warning package.json: License should be a valid SPDX license expression

If this is something we can fix, then maybe we should! :)

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a 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.

@jshcrowthe
Copy link
Contributor Author

@schmidt-sebastian I have addressed the license issues (actually found a couple others per that comment) and so we should be good there.

yarn is a requirement as it is what enables the management of this type of a monorepo. W/o this type of tool, this management is a pain.

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.

@jshcrowthe jshcrowthe merged commit 73a586c into master Oct 7, 2017
@jshcrowthe jshcrowthe deleted the lerna branch October 7, 2017 03:15
@firebase firebase locked and limited conversation to collaborators Oct 26, 2019
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

7 participants