Ncc: Conditional require doesn't include `bufferutil`

[blakeembrey]

Not particularly an issue with either ncc or webpack but spent way too long debugging after I added apollo to my bundle. The issue is that both Apollo and Puppeteer have optional dependencies on bufferutil, however only the first require throws and the second one just returns an empty object. This results in the runtime usage of bufferutil breaking for the second module, since both these are wrapped in a try..catch already (as they're optional dependencies). This is a feature in Webpack and would need to be changed with strictModuleExceptionHandling but would incur a performance penalty.

Wanted to open this for discussion on how this might be been easier debugged or how it should be handled properly. I'm not familiar with Webpack internals either to understand why this couldn't be turned on in a performant way (e.g. by late assigning to the internal module cache, or is this to support circular requires?).

[guybedford]

@blakeembrey thanks for the clear report on this. I think we should look into ensuring we have the strict behaviour both for correctness and debugging, so I would tend to agree this is a bug. Which version of ncc were you using here? Have you tried with the latest beta release? As I believe the behaviours would be different between the two.

[sokra]

strictModuleExceptionHandling but would incur a performance penalty.

"performance penalty" applies only to older v8 versions. If you are using the latest node.js you should be fine.

[styfle]

Does aws-sdk depend on ws?

This issue sounds identical to #413

[blakeembrey]

Definitely an identical issue! I had it from ws + AWS was the second one using buffer util, I think it would have de-duped if they both used as but can check when I’m home.

[blakeembrey]

@styfle Actually, this was a misdiagnosis. It was from apollo-server (specifically subscriptions-transport-ws) which comes with ws which is not de-duped with the ws that was provided by puppeteer since a major version earlier. Not aws-sdk, it just happened to surface when I added aws-sdk to my bundle. Same issue and root cause though, just wrong package culprit.

[blakeembrey]

@sokra Since it's AWS Lambda, it'd be node 10 as the latest.

[Janpot]

Ran into this as well. I could work around it by making bufferutil external and adding a node_modules folder with just bufferutil.

@styfle Trying to read between the lines here, but is there anything blocking the usage of strictModuleExceptionHandling? Would you accept a PR to add it?

[styfle]

I read this thread again and I'm not sure how to reproduce the issue.
Can you explain the steps to reproduce so any PR could also include an integration or unit test?

[Janpot]

@styfle sure, a reproduction is pretty straightforward:

// ./mod.js
throw new Error('kaboom')

// ./index.js
function tryRequire () {
  try {
    require('./mod.js')
    console.log('bad!')
  } catch (err) {
    console.log('good!')
  }
}

tryRequire()
tryRequire()

When you run with node

$ node ./index.js 
  good!
  good!

When you run/build with ncc

$ ncc run -q ./index.js
  good!
  bad!

The solution should be to add strictModuleExceptionHandling: true here, just like next.js does.

[styfle]

@Janpot Perfect! Let's enable strictModuleExceptionHandling. Would you like to create the PR?

[Janpot]

@styfle Sure thing, there you go https://github.com/zeit/ncc/pull/529