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

refactor (functions): restructure project setup #124

Merged
merged 16 commits into from Oct 8, 2022
Merged

Conversation

mkue
Copy link
Contributor

@mkue mkue commented Oct 8, 2022

@andrashee, I think this resolves most of the issues we discussed. Let me know what you think of the new structure.

I think a single firebase.json file makes sense because everything we deploy is in a single Firebase project. We should switch to multiple firebase.json files once we have more than one active project.

FYI, I switched back to using tsc for building the functions because I got the following error when I tried to deploy the functions that were build with webpack.

'@socialincome/shared@^0.1.0' is not in this registry

I removed the @socialincome/shared dependency from the package.json and import the code with relative paths again. Like this, it works at least.

The firebase deploy command does not seem to like monorepos with npm workspaces. We should keep an eye on this issue: firebase/firebase-tools#653.

@github-actions
Copy link

github-actions bot commented Oct 8, 2022

PR Preview Action v1.1.1
🚀 Deployed preview to https://socialincome-san.github.io/public/pr-preview/pr-124/
on branch gh-pages at 2022-10-08 16:31 UTC

@mkue
Copy link
Contributor Author

mkue commented Oct 8, 2022

I would leave the webpack config in the functions/ directory in this PR. We can remove at a later point. Like this, we at least have an example of a working config.

@mkue
Copy link
Contributor Author

mkue commented Oct 8, 2022

Also, we get this annoying warning in the admin front end now:

Screen Shot 2022-10-08 at 16 16 51

The issue is already resolved and the error should go away with the next firebase release.

@github-actions
Copy link

github-actions bot commented Oct 8, 2022

Visit the preview URL for this PR (updated for commit dcdc944):

https://admin-social-income-prod--pr124-restructure-function-9s8cjzz7.web.app

(expires Sat, 15 Oct 2022 16:32:08 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Contributor

@andrashee andrashee left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot for the patience on this one.

One thing which I don't fully understand yet. Why do we need the webpack packages when we do it with local references and not using the package?

import { IconButton, Tooltip } from '@mui/material';
import { getFunctions, httpsCallable } from 'firebase/functions';

export default function CallDummyFunctionButton() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🥳


// algolia search
export const ALGOLIA_APPLICATION_ID = process.env.REACT_APP_ADMIN_ALGOLIA_APPLICATION_ID;
export const ALGOLIA_SEARCH_KEY = process.env.REACT_APP_ADMIN_ALGOLIA_SEARCH_KEY;

// production envs
export const FIREBASE_CONFIG = {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"scripts": {
"build": "tsc",
"build:watch": "tsc --watch",
"build:webpack": "webpack --config webpack.config.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

for what are we requiring now the webpack? For the final deployment?

functions/src/etl/importPostfinanceBalance.ts Outdated Show resolved Hide resolved
@@ -3,6 +3,6 @@
"version": "0.1.0",
"private": true,
"devDependencies": {
"@camberi/firecms": "2.0.0-alpha.30"
"@camberi/firecms": "2.0.0-alpha.37"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice if the shared package doesn't have a dependency to the firecms.

What do you think of the approach I had in https://github.com/socialincome-san/public/pull/86/files#diff-eb15213d223e369551834a6ab5f0831a9a4484dfe9a44dabb2e29f91898f84ac where I copied the EntityReference implementation into the shared package? It's the only dependency we use, and then we can get rid of this one.

Makes the shared and therefore functions package much slimmer and faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but isn't that a problem when the dependent object changes? If it's just a devDependency, I don't see the problem. It will be in your node_modules folder anyway because it's being used in another project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Feels conceptually wrong to me that the functions have a compilation dependency to the firecms, but we can still iterate over it later.

npm ERR! Error: Missing script: "functions:test"

To see a list of scripts, run:
  npm run 
npm ERR!   in workspace: @socialincome/admin@0.1.0 
npm ERR!   at location: /home/node/admin 
npm ERR! Missing script: functions:test

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/node/.user/.npm/_logs/2022-10-08T15_06_34_214Z-debug-0.log
make: *** [functions-test] Error 1

~/code/public restructure-functions *1 ?2 ❯ make functions-test   

should that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my bad cached container.

@andrashee
Copy link
Contributor

I would leave the webpack config in the functions/ directory in this PR. We can remove at a later point. Like this, we at least have an example of a working config.

Ah, haven't seen this comment during my review. Make sense 👍

@mkue mkue marked this pull request as ready for review October 8, 2022 14:54
Copy link
Contributor

@andrashee andrashee left a comment

Choose a reason for hiding this comment

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

very nice!

@mkue mkue requested a review from renestalder as a code owner October 8, 2022 15:48
@mkue mkue changed the title Restructure functions [Functions] Restructure project setup Oct 8, 2022
@mkue mkue changed the title [Functions] Restructure project setup refactor (functions): restructure project setup Oct 8, 2022
@mkue mkue merged commit 5eee5a7 into main Oct 8, 2022
@mkue mkue deleted the restructure-functions branch October 8, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants