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
fix(page): fallback to default in exposeFunction when using imported module #6365
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@joshgrime thanks for your PR and sorry about the delay in responding. I'm 50/50 on this one to be honest - I'm not sure if actually asking users to pass @mathiasbynens would be good to have your opinion here, WDYT? |
@jackfranklin thanks for the response. I understand the deliberation; I went through it a bit myself, hence adding the quick fix at the end. My thinking was that since I couldn't forsee any significant downsides to having the fallback option, and not having it could lead to errors/confusion as in the issues raised, it was worth adding it in. Really interested to hear of any negative side-effects or reasoning I haven't considered, especially as this is my first contribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch and the detailed description!
I'm ok with letting Puppeteer handle it, as this patch does. The current patch LGTM % a suggestion. Would you mind adding a test for this new behavior as well?
With those resolved, we can merge this.
src/common/Page.ts
Outdated
typeof puppeteerFunction['default'] === 'function' | ||
) { | ||
puppeteerFunction = puppeteerFunction['default']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof puppeteerFunction['default'] === 'function' | |
) { | |
puppeteerFunction = puppeteerFunction['default']; | |
typeof puppeteerFunction.default === 'function' | |
) { | |
puppeteerFunction = puppeteerFunction.default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mathiasbynens , thanks for your comments and suggestions.
I've added the test and is passing for me. Had to use expect error next line as it's expecting a function rather than an object:
it('should fallback to default export when passed a module object', async () => {
const { page, server } = getTestState();
await page.goto(server.EMPTY_PAGE);
var moduleObject = {
default: function(a, b) {
return a * b;
}
};
// @ts-expect-error
await page.exposeFunction('compute', moduleObject);
const result = await page.evaluate(async function () {
return await globalThis.compute(9, 4);
});
expect(result).toBe(36);
});
The changes to dot syntax in Page.ts is throwing an error for me though, please let me know if you prefer to ignore the line or keep the [ ] syntax.
src/common/Page.ts:1058:32 - error TS2339: Property 'default' does not exist on type 'never'.
1058 typeof puppeteerFunction.default === 'function'
Howdy, any movement here? Thanks! |
03a4ca1
to
d4b17bd
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@joshgrime I updated the change to address typing problems and added documentation. Let me know if this still looks good to you and I'm happy to get this landed! |
@googlebot I consent. |
Looks good to me @jschfflr, thanks for picking this up |
Happy to help. Let's wait for @mathiasbynens review to get this landed :) |
Have been addressed in the latest version.
…mported module (puppeteer#6365)" This reverts commit 44c9ec6.
I came across the same issue as #5586 and #6293 and decided to take a look.
The problem appears to be that if you pass
page.exposeFunction
an imported module which usesexport default
, thenthis._pageBindings.get(name)
will return an object rather than a function as expected.myModule.js
App.js
puppeteer/common/Page.js
Inspecting
const result = await this._pageBindings.get(name)
, it is actually an object with a default property:I've added the following fallback to
exposeFunction
, which will use the 'default' if there's a function defined for it:Alternative fix
An alternative way to get around this with no changes is, if you are using a default export, to make sure you pass the default function rather than the whole module to
page.exposeFunction
. For example:App.js
For me it's preferable to include the fallback. I think that this behaviour is more in-line with what you expect from default exported modules.