Web Frameworks support: Use ES6 module syntax and use main file in express integration #5248
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
simonnepomuk
changed the title
|
@jamesdaniels |
|
@simonnepomuk thanks for this, I'll pull down on to my machine and experiment some more |
Will do. |
| await writeFile( | ||
| join(functionsDist, "server.js"), | ||
| `const { onRequest } = require('firebase-functions/v2/https'); | ||
| const server = import('firebase-frameworks'); | ||
| exports.ssr = onRequest((req, res) => server.then(it => it.handle(req, res))); | ||
| ` | ||
| `import { onRequest } from 'firebase-functions/v2/https'; | ||
| const server = import('firebase-frameworks'); | ||
| export const ssr = onRequest((req, res) => server.then(it => it.handle(req, res)));` | ||
| ); |
There was a problem hiding this comment.
Agree with your comment on the PR that we will need some logic to retain the old CJS behavior as well.
Should be straightforward with something like if (packageJson.type === 'module') ....
We could extract the snippets to consts, keep them as inline literals, or even bring them out into the templates folder like the TODO comment suggests, but I'll defer to @jamesdaniels on that one.
There was a problem hiding this comment.
I could see
const importStatements = packageJson.type === 'module' ?
"import { onRequest } from 'firebase-functions/v2/https';" :
"const { onRequest } = require('firebase-functions/v2/https');"`if we wanted to DRY it up
const importsFrom = [["onRequest"], "firebase-functions/v2/https"];
const importStatements = importsFrom.map(([imports, from]) => packageJson.type === "module" ? ... : ...);
...There was a problem hiding this comment.
We'll need the same for the export, right? Might be easier/clearer to just repeat the whole block.
There was a problem hiding this comment.
SG, let's just repeat the block
| return ( | ||
| bootstrapScript + | ||
| ";\nexports.handle = async (req, res) => (await bootstrap).handle(req, res);" | ||
| ";\nexport const handle = async (req, res) => (await bootstrap).handle(req, res);" |
There was a problem hiding this comment.
Same comment regarding CJS support
| return ( | ||
| bootstrapScript + | ||
| ";\nexports.handle = async (req, res) => (await bootstrap).app(req, res);" | ||
| ";\nexport const handle = async (req, res) => (await bootstrap).app(req, res);" |
There was a problem hiding this comment.
Same comment regarding CJS support
|
@simonnepomuk Let me know if you're still up for getting this merged. If not, we can adapt your PR and move forward ourselves, with attribution back to your work. |
Hey @austincrim, thx for the review. My interest in using firebase dwindled over the last months because of some regulatory issues. Feel free to move forward. |
3 participants
Description
This should fix two issues:
package.jsonlies). Actually, it should import the file specified in themainproperty of thepackage.json"type": "module"inpackage.jsonthe generated function fails to execute since the generated code contains arequire(...)statement.Should close:
Scenarios Tested
I linked the
firebase-toolsto my branch usingnpm linkand ran the tests in the firebase-framework-tools repo. Is this how it should be done? I needed to replace the site ID in a couple of files to a site ID which I have access to.This was also tested in the branch of the web framework support PR of my SvelteKit adapter with
emulators:startanddeploy.