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

feat(examples/firebase): deploy to Cloud Functions for Firebase #3398

Closed
wants to merge 2 commits into from

Conversation

penx
Copy link
Contributor

@penx penx commented Jun 6, 2022

Deploy to firebase functions and hosting

Closes: Discussion at #1811 (comment)

  • Docs
  • Tests (I'd happily add Playwright tests if you want these on examples)

Testing Strategy:

  • Testing locally against emulator with JS enabled and disabled
  • Testing locally against production with JS enabled and disabled
  • Following deploy instructions and testing remote with JS enabled and disabled

"react": "^17.0.2",
"react-dom": "^17.0.2"
"react-dom": "^17.0.2",
"remix-google-cloud-functions": "0.0.1"
Copy link
Contributor Author

@penx penx Jun 6, 2022

Choose a reason for hiding this comment

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

I've created and published this adapter separately, I can either:

  • manage this myself in its own repository
  • contribute it back as @remix-run/google-cloud-functions

I've opened up a separate PR for this at #3397

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge #3397 first before continuing with this.

@MichaelDeBoey MichaelDeBoey marked this pull request as draft June 6, 2022 13:09
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Putting this in draft until #3397 is merged

"react": "^17.0.2",
"react-dom": "^17.0.2"
"react-dom": "^17.0.2",
"remix-google-cloud-functions": "0.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Let's merge #3397 first before continuing with this.

@penx
Copy link
Contributor Author

penx commented Jun 8, 2022

Now that examples/firebase-auth-firestore is including lots of other firebase features, I think it should be renamed examples/firebase

@MichaelDeBoey
Copy link
Member

@penx If you want, you can do that in another PR

@imadbz
Copy link

imadbz commented Jun 9, 2022

Can I use this right now?

@penx
Copy link
Contributor Author

penx commented Aug 24, 2022

Putting this in draft until #3397 is merged

#3397 was closed in favour of #2266 which has stalled for 2 months due to new adapters not being high on the priority list.

I think it would be worth revisiting this example without waiting for #2266 as it is something that comes up by users e.g. in Discord.

I would suggest either:

  • keeping the example as-is, using my remix-google-cloud-functions (previously remix-firebase) unofficial adapter that I published to npm (happy to hand over ownership)
  • add the adapter in to the example code (in examples/firebase-auth-firestore/functions/adapter.ts) and use this instead

Any suggestions/preference @MichaelDeBoey @kentcdodds?

@penx
Copy link
Contributor Author

penx commented Aug 24, 2022

Can I use this right now?

@imadbz sorry for the late reply, yes remix-google-cloud-functions (previously remix-firebase) should be good to use now as per the example code in this PR

@penx penx requested a review from MichaelDeBoey August 24, 2022 13:45
@penx penx marked this pull request as ready for review August 24, 2022 13:45
@MichaelDeBoey MichaelDeBoey changed the title feat(examples/firebase-auth-firestore): deploy to firebase functions feat(examples/firebase): deploy to Cloud Functions for Firebase Aug 24, 2022
@MichaelDeBoey
Copy link
Member

@penx Please rebase your PR onto latest main & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Aug 24, 2022
@MichaelDeBoey MichaelDeBoey requested a review from machour August 24, 2022 17:08
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

I'm not 100% if we want to merge this tbh, as we'll most likely create an official template once #2266 is merged 🤔

@penx penx force-pushed the firebase-functions branch from 7acb54e to b2b4cb4 Compare August 24, 2022 17:33
@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2022

⚠️ No Changeset found

Latest commit: b2b4cb4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@penx
Copy link
Contributor Author

penx commented Aug 24, 2022

I'm not 100% if we want to merge this tbh, as we'll most likely create an official template once #2266 is merged 🤔

Once #2266 is merged and there is an official template, wouldn't it still make sense for the example to be using the adapter too so that it's a "one-stop example for all things firebase"?

@penx penx requested review from MichaelDeBoey and removed request for machour August 24, 2022 17:37
@penx
Copy link
Contributor Author

penx commented Aug 24, 2022

Screenshot 2022-08-24 at 18 38 03

GitHub for some reason removed @machour automatically when I re-requested review from @MichaelDeBoey. I'm not sure why it shows me as removing @machour as I don't have access to do that 😄

@MichaelDeBoey MichaelDeBoey requested a review from machour August 24, 2022 17:43
@MichaelDeBoey MichaelDeBoey removed the needs-response We need a response from the original author about this issue/PR label Aug 24, 2022
@chaance
Copy link
Collaborator

chaance commented Sep 15, 2022

FYI we are migrating all examples over to https://github.com/remix-run/examples. If you are still interested in including this example, feel free to open a new PR there at your convenience 🙂

@penx
Copy link
Contributor Author

penx commented Sep 16, 2022

Reopened as remix-run/examples#5

@penx penx deleted the firebase-functions branch September 16, 2022 13:38
MichaelDeBoey pushed a commit to penx/examples that referenced this pull request Sep 28, 2022
penx added a commit to penx/examples that referenced this pull request Oct 4, 2022
penx added a commit to penx/examples that referenced this pull request Oct 4, 2022
penx added a commit to penx/examples that referenced this pull request Oct 31, 2022
penx added a commit to penx/examples that referenced this pull request Oct 31, 2022
penx added a commit to penx/examples that referenced this pull request Jan 24, 2023
penx added a commit to penx/examples that referenced this pull request Jan 25, 2023
An exact version is needed as Firebase uses this to determine which version of node to use for the functions when you run `firebase deploy`

Node 14 is a mismatch to root .nvmrc which is set to 16

This will result in errors from both yarn and npm install.

```
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: undefined,
npm WARN EBADENGINE   required: { node: '14' },
npm WARN EBADENGINE   current: { node: 'v16.19.0', npm: '8.19.3' }
npm WARN EBADENGINE }
```

This could be skipped using `yarn  --ignore-engines`, but I don't think that's a great experience for an example project.

Previous discussion.

remix-run/remix#3398 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants