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

Data Connect Product #8480

Merged
merged 91 commits into from
Sep 24, 2024
Merged

Data Connect Product #8480

merged 91 commits into from
Sep 24, 2024

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented Sep 4, 2024

Added Data Connect product (v0.1.0-beta)

maneesht and others added 30 commits May 7, 2024 17:09
commit d9ed6147cb5a1b34408342193896d9153a90898c
Author: Maneesh Tewani <mtewani@google.com>
Date:   Thu May 9 10:09:49 2024 -0700

    Fixed issue where transport is undefined
* Addressed comments

* Included api report

* Removed console.log
* Began reporting client sdk information
yarn.lock Outdated Show resolved Hide resolved
Copy link
Contributor

@DellaBitta DellaBitta left a comment

Choose a reason for hiding this comment

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

I believe that all of the files added in this PR that can have copyright notice headers should be copyright 2024 since they are new files.

@@ -0,0 +1,34 @@
/**
* @license
* Copyright 2017 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, reviving this comment. Many of these files have copyrights from varying years. Shouldn't they all be 2024?

// },
// external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`)),
// onwarn: onWarn
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

// `Provided AppCheck credentials for the app named "${this.appName_}" ` +
// 'are invalid. This usually indicates your app was not initialized correctly.'
// );
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.


import fs from 'fs';
import * as path from 'path';
// curl localhost:3628/setupSchema -X POST -d '{
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is still needed the maybe add a comment to what it's used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Contributor

@DellaBitta DellaBitta left a comment

Choose a reason for hiding this comment

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

Please fill in a PR description based on the template. Thanks!

packages/firebase/data-connect/index.ts Outdated Show resolved Hide resolved
packages/data-connect/src/index.ts Show resolved Hide resolved
Copy link
Contributor

@DellaBitta DellaBitta left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@maneesht maneesht merged commit beaa4df into main Sep 24, 2024
41 of 43 checks passed
@maneesht maneesht deleted the dataconnect branch September 24, 2024 08:33
@google-oss-bot google-oss-bot mentioned this pull request Sep 30, 2024
@firebase firebase locked and limited conversation to collaborators Oct 25, 2024
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.

4 participants