search

prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.28k stars 2.05k forks source link

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'makeBidRequests') #8374

Closed audiencerun closed 2 years ago

audiencerun commented 2 years ago

Type of issue

Bug

Description

We wanted to update our version of Prebid.js from 6.12.0 to the latest version 6.22.0. Once updated, we noticed this error in the console

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'makeBidRequests')

IsawNCGP

After several tests, it seems that we have the error when switching to Prebid.js starting 6.17.0 and higher. The possibilities we see in the diffs may be related to the adapterManager.js file via Prebid core & PBS adapter: better support for server-side stored impressions by dgirardi · Pull Request #8154 · prebid/Prebid.js ?

or to the webpack upgrade from 3.0.0 to 5.70.0 via Build system: upgrade webpack by dgirardi · Pull Request #7935 · prebid/Prebid.js ?

we have tried some fixes, here we import adapterManager.js via a require

Prebid.js/auction.js at e07025247a81bf8c04501de8c66d817143cc30d1 · prebid/Prebid.js

we added some logs to understand, and replaced require with an import

adapterManager => import adapterManager from './adapterManager.js';

_adapterManager => const _adapterManager = require('./adapterManager.js').default;

We see that on the second call of callBids, adapterManager becomes undefined when imported with a require while it is not the case when imported with an import.

But we don't know if this is the real cause of the error.

image

Steps to reproduce

Test page

https://7buoei.vercel.app/test.html

Expected results

No errors in console

Actual results

Error in console

Platform details

dependency/environment version
Prebid.js 6.22.0
Prebid.js global 'paubjs'
npm 8.5.5
node 16.15.0
browser Chrome/100.0.4896.75
OS macOS 10.15.7

Feel free if you need more details.

coreyb-cbs commented 2 years ago

I encountered the exact same issue yesterday while attempting to upgrade from Prebid 6.8 to 6.22 and, like you, I also noticed that the issue is resolved if the require is changed to an import in auction.js.

dgirardi commented 2 years ago

If you're using Webpack, can you post its version and the configuration you're using for it?

audiencerun commented 2 years ago

Hi @dgirardi

We use Prebid.js from an external script and not directly as a package. It's built locally from the Prebid.js repo and available at https://ac.audiencerun.com/j/prebid/pre.js.

This script is injected through our project in the page via <script></script>. Our project is using webpack 4.46.0. Could this be related?

module.exports = {
    ...
    mode: "production",
    output: {
        path: BUILD_PATH,
        jsonpFunction: JSONP_FUNCTION,
        chunkFilename: CHUNK_FILENAME,
        filename: FILENAME
    },
    module: {
        rules: [
            ...utils.styleLoaders(options),
            ...utils.assetsLoaders(options),
            ...utils.jsLoaders(options)
        ]
    },
    optimization: {
        splitChunks: {
            name: false
        }
    }
};
dgirardi commented 2 years ago

just to confirm - can you also post the contents of utils.jsLoaders ? (or the portions that are relevant to Prebid - there should be a special case for it in there)

right now I think this is a difference in how Webpack treats require between versions 4 and 5, with the latter being able to resolve some recently introduced circular import that trips v4.

I am now converting every require in the codebase to import, which I think is a good idea anyway, but I'm not sure it's a great solution long term - there could be differences in how that gets translated by different versions as well. We should merge https://github.com/prebid/Prebid.js/pull/8175 soon which would allow you to avoid compiling Prebid which should bypass this issue completely, but minification may not be as efficient.

audiencerun commented 2 years ago

Here is the content of jsLoaders

jsLoaders: (options = {}) => [
    {
        test: /\.js?$/,
        exclude: /(node_modules|bower_components)/,
        use: [
            {
                loader: "babel-loader"
            },
            {
                loader: StringReplacePlugin.replace({
                    replacements: replaceMacros(options)
                })
            }
        ]
    }
]
dgirardi commented 2 years ago

huh - Prebid needs its own babel transpiler (as noted in the README), I'm now confused as to how you had it working before. Is it because it's not under node_modules and you have some treatment for it in replaceMacros?

What happens if you add this rule? (assuming it has 'prebid.js' somewhere in its path)

 {
        test: /.js$/,
        include: new RegExp(`\\${path.sep}prebid\\.js`),
        use: {
          loader: 'babel-loader',
          // presets and plugins for Prebid.js must be manually specified separate from your other babel rule.
          // this can be accomplished by requiring prebid's .babelrc.js file (requires Babel 7 and Node v8.9.0+)
          // as of Prebid 6, babelrc.js only targets modern browsers. One can change the targets and build for
          // older browsers if they prefer, but integration tests on ie11 were removed in Prebid.js 6.0
          options: require('prebid.js/.babelrc.js')
        }
audiencerun commented 2 years ago

Just to confirm Prebid.js project is completely separated from our project.

We use Prebid.js compiled file (prebid.js) after building it through https://github.com/prebid/Prebid.js#build-optimization and not as an npm dependency as mentioned here https://github.com/prebid/Prebid.js#usage-as-a-npm-dependency.

replaceMacros is just replacing string macros present in our project code.

dgirardi commented 2 years ago

OK - that makes more sense. So now the question is, why would that be different from how we are building it - what steps are you using to build (including your --module arguments)?

audiencerun commented 2 years ago

Build steps

dgirardi commented 2 years ago

and your modules.json?

dgirardi commented 2 years ago

Is it possible that your dependencies are out of sync - did you npm ci before building?

audiencerun commented 2 years ago

modules.json

[ 'improvedigitalBidAdapter', 'smartadserverBidAdapter', 'criteoBidAdapter', 'consentManagement', 'appnexusBidAdapter', 'pubmaticBidAdapter', 'rubiconBidAdapter', 'smartyadsBidAdapter', 'onetagBidAdapter', 'eplanningBidAdapter', 'currency', 'unrulyBidAdapter', 'sovrnBidAdapter', 'rhythmoneBidAdapter', 'schain', 'emx_digitalBidAdapter', 'userId', 'uid2IdSystem', 'criteoIdSystem', 'sharedIdSystem', 'sharethroughBidAdapter', 'yieldmoBidAdapter', 'adkernelBidAdapter', 'adyoulikeBidAdapter', 'amxBidAdapter', 'vidoomyBidAdapter', 'richaudienceBidAdapter', 'adagioBidAdapter', 'luponmediaBidAdapter', 'axonixBidAdapter', 'connectadBidAdapter', 'id5IdSystem', 'amxIdSystem', 's2sTesting', 'prebidServerBidAdapter' ]

Is it possible that your dependencies are out of sync - did you npm ci before building?

We make sure that dependencies are always up to date before building. Until then, everything worked fine.

dgirardi commented 2 years ago

I am unable to reproduce - what Node version are you using?

I have the following:

demetrio@pbws:~/src/Prebid.js$ node --version
v17.5.0
demetrio@pbws:~/src/Prebid.js$ git status
HEAD detached at 6.22.0
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   package.json

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        modules.json

no changes added to commit (use "git add" and/or "git commit -a")
demetrio@pbws:~/src/Prebid.js$ cat modules.json 
["improvedigitalBidAdapter", "smartadserverBidAdapter", "criteoBidAdapter", "consentManagement", "appnexusBidAdapter", "pubmaticBidAdapter", "rubiconBidAdapter", "smartyadsBidAdapter", "onetagBidAdapter", "eplanningBidAdapter", "currency", "unrulyBidAdapter", "sovrnBidAdapter", "rhythmoneBidAdapter", "schain", "emx_digitalBidAdapter", "userId", "uid2IdSystem", "criteoIdSystem", "sharedIdSystem", "sharethroughBidAdapter", "yieldmoBidAdapter", "adkernelBidAdapter", "adyoulikeBidAdapter", "amxBidAdapter", "vidoomyBidAdapter", "richaudienceBidAdapter", "adagioBidAdapter", "luponmediaBidAdapter", "axonixBidAdapter", "connectadBidAdapter", "id5IdSystem", "amxIdSystem", "s2sTesting", "prebidServerBidAdapter"]
diff --git a/package.json b/package.json
index 1318c1ff..ede5f2ea 100644
--- a/package.json
+++ b/package.json
@@ -22,7 +22,7 @@
     "header bidding",
     "prebid"
   ],
-  "globalVarName": "pbjs",
+  "globalVarName": "paubjs",
   "author": "the prebid.js contributors",
   "license": "Apache-2.0",
   "engines": {

running npm ci && gulp build --modules=modules.json produces this bundle, which is working for me:

prebid.zip

adapterManagerDef

audiencerun commented 2 years ago

Using node 16.15.0

There seems to be no more errors with your file 🤔

dgirardi commented 2 years ago

I tried with 16.15.0 and the minimum required 12.x, no difference - still works for me. I'm now properly confused. Would you be willing to make your version of the Prebid repo available as a fork for me to mess with it?

audiencerun commented 2 years ago

We don't have a repo specially for Prebid, as we have a job to get a fresh install from https://github.com/prebid/Prebid.js, then as you have done switch to the wanted version, filling modules.json, replacing globalVarName and finally install packages and run the build.

dgirardi commented 2 years ago

If the job happens to be (or can be made into) a script that reliably produces that error on your system that would help - I have to believe there's some other difference in our setups, otherwise the only thing left to blame is the OS which seems like a stretch.

I will follow up with a PR that replaces require into import - but I wouldn't be comfortable in trusting that to solve the issue if we can't find it; there may other similar problems in your output that could stay hidden until you put it in production.

audiencerun commented 2 years ago

Thanks a lot for your help. Same, we are also trying to find out from our side where it can come from. We'll keep you posted!

dgirardi commented 2 years ago

https://github.com/prebid/Prebid.js/pull/8379 converts all requires into imports - since I am unable to reproduce the issue here, would either @audiencerun or @coreyb-cbs be available to try it? thanks!

audiencerun commented 2 years ago

Hi @dgirardi

Yes, of course.

➜  Prebid.js git:(master) ✗ gh pr checkout 8379
remote: Enumerating objects: 24, done.
remote: Counting objects: 100% (24/24), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 24 (delta 19), reused 24 (delta 19), pack-reused 0
Unpacking objects: 100% (24/24), done.
From https://github.com/prebid/Prebid.js
 * [new ref]           refs/pull/8379/head -> no-require
Switched to branch 'no-require'

No more errors in the console. Thanks! ✅

hKZTmeOR

kdBQkfOJ

Just out of curiosity I compared the file you provided in https://github.com/prebid/Prebid.js/issues/8374#issuecomment-1118848785 with the built one on our side and I don't think they are built the same way. Same situation when compared with https://cdn.jsdelivr.net/npm/prebid.js@latest/dist/not-for-prod/prebid.js

Our file

same thing on https://cdn.jsdelivr.net/npm/prebid.js@latest/dist/not-for-prod/prebid.js

Your file

dgirardi commented 2 years ago

The offending files in the OP are no longer served - can you attach one here for reference? Does the bundle layout really change if you switch to that PR?

dgirardi commented 2 years ago

to me that looks like the webpack or webpack configuration used in the broken build is not the one in the Prebid repo. I don't know what could cause it - maybe an odd NODE_PATH resolution order, or some caching I'm not aware of?

dgirardi commented 2 years ago

Scratch that - it's a red herring. I messed up - my local 6.22.0 tag, that I used to build the file, was not the same as the 6.22.0 tag in origin - it was my version of webpack being different. I must have set that up at some point and forgot about it.

This is the file I get with the actual 6.22.0 version - it still works fine for me.

prebid.zip

audiencerun commented 2 years ago

Using your Prebid.js file, uploaded to https://7buoei.vercel.app/prebid-6.22.0@dgirardi.js

I still have the error

aoOEjFLi

dgirardi commented 2 years ago

I can reproduce now, but only if I go through a European VPN - investigating.

dgirardi commented 2 years ago

@audiencerun I have a lead, but it will take some time for me to reproduce locally to confirm - it might be easier for you:

I think the VPN might not be important, but the timing is. You have some code that does:

      loadPrebid(e, t) {
        if (this.window.paubjs.libLoaded) return void (Object(c.c) (e) && e());
        let i;
        i = s.c ? 'https://prebid.local.dev.blackcover.fr:9001/static/assets/audiencerun/dev/prebid.js?42' : s.a ? 'https://ac.audiencerun.com/predevelop/j/prebid/pre.js?42' : 'https://ac.audiencerun.com/j/prebid/pre.js?42',
        Object(p.a) (i, () =>{
          Object(c.c) (e) && e()
        }, t, this.window)
      }

What I see is that that gets called twice. The second time, on my system - without VPN - it short-circuits on if (this.window.paubjs.libLoaded). With the VPN it doesn't and it goes on to show the exception. makeBidRequests is called twice, and it's only on the second that it fails as undefined.

So now I'm thinking that require is broken if you load Prebid twice (it kinda maybe makes sense because Prebid loads depend on the global pbjsChunk).

Could you try changing that if (this.window.paubjs.libLoaded) with something like if(window.paubJsLoaded) then [shortCircuit] else window.paubJsLoaded = true and see if it makes a difference for you?

dgirardi commented 2 years ago

The loading / chunking mechanism changed with the Webpack upgrade, so that also checks out. I'll see if I can get the timing right to reproduce it locally.

dgirardi commented 2 years ago

If I redirect to this version (which is modified to check for libLoaded):

https://s3.amazonaws.com/files.prebid.org/test/dgirardi/prebid-undef-test.js

I can no longer reproduce the issue. Still not completely confident (as always with race conditions) - if you can confirm that fixes it might be worth doing in master.

audiencerun commented 2 years ago

Just used your file https://s3.amazonaws.com/files.prebid.org/test/dgirardi/prebid-undef-test.js on https://7buoei.vercel.app/test.html

No more errors in the console ✅

vCsVgJLh

dgirardi commented 2 years ago

I did some more digging, and I think the proper fix is to prevent Prebid from loading twice - I would not trust the switch to import to solve this. (Paging @coreyb-cbs as well).

The issue with the proper fix is that it might be a breaking change if we do it within Prebid. I'll plan to get it in for 7 at least.

What happens is:

That is, import happens to generate a lazy dereference in this case. But not in general; these idioms would have the same problem:

import * as am from './src/adapterManager.js';
const adapterManager = am['default'];
function callBids() {
   adapterManager.makeBidRequests(...)
}

// or when there's no option to be lazy

import {someSymbol} from './someModule';
someSymbol();

The bottom line is that even if everything happens to work right now after switching to import, it could easily break (for the second load of Prebid) just by shuffling around their order, introduction of new circular imports, or refactoring of imports to forms that will appear equivalent.

dgirardi commented 2 years ago

There might also be a way deep in the internals of Webpack to force it to "lose" the registry on the second load - that would be a cleaner fix.

dgirardi commented 2 years ago

The problem is that webpack will re-use pbjsGlobal if it's already defined:

https://github.com/webpack/webpack/blob/5a26b7c4a923c42022ca97fe9cbf46907a20d6d8/lib/web/JsonpChunkLoadingRuntimeModule.js#L425

I don't see a way to alter that behavior without duplicating all that functionality in a separate plugin.

We can generate bundles that clear or scope down pbjsGlobal, but that would not work for people not using the bundle - maybe that's good enough?

dgirardi commented 2 years ago

Discussed this today; the plan is to ship a change in Prebid 7 that would prevent the second copy of prebid from running if it's using the same globalVarName as an instance of Prebid that's already running.

If you're using an earlier version of Prebid, and you're affected by this issue, you should prevent the second loading of Prebid yourself. Note that even in 6.12 and earlier, the Prebid codebase assumes one instance per globalVarName - just because it doesn't show obvious exceptions doesn't mean it works if you load it multiple times; we never tested that scenario.