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

fix(page): fallback to default in exposeFunction when using imported module #6365

Merged
merged 8 commits into from Sep 29, 2021

Conversation

joshgrime
Copy link
Contributor

@joshgrime joshgrime commented Aug 27, 2020

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 uses export default, then this._pageBindings.get(name) will return an object rather than a function as expected.

myModule.js

function parseData(options){
    console.log(options);
}

export default parseData;

App.js

var plugin = await import('./myModule.js');
var options = {/*     */};
await page.exposeFunction('myPlugin', plugin);
var data = await page.evaluate(async options => {
                var payload = await myPlugin(options);
                return payload;
 }, options);

puppeteer/common/Page.js

async _onBindingCalled(event) {
        const { name, seq, args } = JSON.parse(event.payload);
        let expression = null;
        try {
            const result = await this._pageBindings.get(name)(...args);
...
}

Inspecting const result = await this._pageBindings.get(name), it is actually an object with a default property:

console.log(result);
// [Module] { default: [Function: parseData] }

I've added the following fallback to exposeFunction, which will use the 'default' if there's a function defined for it:

if (typeof puppeteerFunction !== 'function' &&
      typeof puppeteerFunction['default'] === 'function') {
      puppeteerFunction = puppeteerFunction['default'];
 }

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

//From:
await page.exposeFunction('myPlugin', plugin);
//To:
await page.exposeFunction('myPlugin', plugin.default);

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.

@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@joshgrime
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jackfranklin
Copy link
Collaborator

@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 foo.default in this case is more reasonable rather than hide it on the Puppeteer side. But I can also see the reasoning to have Puppeteer do it.

@mathiasbynens would be good to have your opinion here, WDYT?

@joshgrime
Copy link
Contributor Author

@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

Copy link
Member

@mathiasbynens mathiasbynens left a 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.

Comment on lines 1058 to 1060
typeof puppeteerFunction['default'] === 'function'
) {
puppeteerFunction = puppeteerFunction['default'];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
typeof puppeteerFunction['default'] === 'function'
) {
puppeteerFunction = puppeteerFunction['default'];
typeof puppeteerFunction.default === 'function'
) {
puppeteerFunction = puppeteerFunction.default;

Copy link
Contributor Author

@joshgrime joshgrime Sep 14, 2020

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'

@Quentin-M
Copy link

Howdy, any movement here? Thanks!

@google-cla
Copy link

google-cla bot commented Sep 15, 2021

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Sep 15, 2021
@jschfflr
Copy link
Contributor

@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!

@jschfflr
Copy link
Contributor

@googlebot I consent.

@jschfflr jschfflr enabled auto-merge (squash) September 15, 2021 19:55
@jschfflr jschfflr enabled auto-merge (squash) September 15, 2021 19:59
@joshgrime
Copy link
Contributor Author

Looks good to me @jschfflr, thanks for picking this up

@jschfflr
Copy link
Contributor

Happy to help. Let's wait for @mathiasbynens review to get this landed :)

@jschfflr jschfflr dismissed mathiasbynens’s stale review September 29, 2021 16:32

Have been addressed in the latest version.

@jschfflr jschfflr merged commit 44c9ec6 into puppeteer:main Sep 29, 2021
DelysidBot added a commit to DelysidBot/puppeteer that referenced this pull request Nov 11, 2021
This was referenced May 30, 2022
This was referenced May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants