dynamic import doesn't capture outer context

[dnalborczyk]

• Rollup Version: v1.20.3
• Operating System (or Browser): macOS
• Node Version: v12.9.1

output.format: cjs

How Do We Reproduce?

class Foo {
  constructor() {
    this._filename = './some-path.js'
  }

  async bar() {
    await import(this._filename)
  }
}

new Foo().bar()

is transpiled to something like:

class Foo {
  constructor() {
    this._filename = './some-path.js'
  }

  async bar() {
    await new Promise(function (resolve) { resolve(require(this._filename)) })
  }
}

new Foo().bar()

Expected Behavior

it should potentially be transpiled to something like:

// notice the arrow function
new Promise((resolve) => { resolve(require(this._filename)) })

to capture the outer context.

Actual Behavior

throws exception when running bundled code:
TypeError: Cannot read property '_filename' of undefined

https://rollupjs.org/repl/?version=1.20.3&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNsYXNzJTIwRm9vJTIwJTdCJTVDbiUyMCUyMGNvbnN0cnVjdG9yKCklMjAlN0IlNUNuJTIwJTIwJTIwJTIwdGhpcy5fZmlsZW5hbWUlMjAlM0QlMjAnLiUyRmJhci5qcyclNUNuJTIwJTIwJTdEJTVDbiU1Q24lMjAlMjBhc3luYyUyMGJhcigpJTIwJTdCJTVDbiUyMCUyMCUyMCUyMGF3YWl0JTIwaW1wb3J0KHRoaXMuX2ZpbGVuYW1lKSU1Q24lMjAlMjAlN0QlNUNuJTdEJTVDbiU1Q25uZXclMjBGb28oKS5iYXIoKSUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMmJhci5qcy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBkZWZhdWx0JTIwJTdCJTdEJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmNqcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

edit: added repl, cjs note, some clarifications

[lukastaegert]

Thanks for this issue. Yes the arrow function would be the easiest solution. At the moment, we try to maintain at least some ES5 compatibility, but I think an ES5 approach (which would probably introduce a new variable) would have its own problems. A compromise could be for now to check if the expression inside a dynamic import uses an unchecked this and only add an arrow function in these cases. This would still generate ES6 code, but only in the edge case scenario.

[dnalborczyk]

thank you @lukastaegert !

out of curiosity, why is ES5 compatibility desired? modules itself (when bundled to esm) are part of ES6 as well as Promises. For older browser support? I would think that it makes everything harder to implement and to maintain.

just some food for thought if ES5 is desired rollup could possibly wrap the import expression in a function scope, and call it, roughly like this:

const dynImpExpr = function() {
  return this.val
}.bind(this)
    
return new Promise(function (res)  {
  res(require(dynImpExpr()))
})

but like you mentioned, it might introduce its own problems.

[dnalborczyk]

quick workaround for anyone experiencing this issue until fixed:

async bar() {
  const filename = this._filename
  await import(filename)
}

[lukastaegert]

This will be resolved in #4215 at last