feat(commonjs): export properties defined using Object.defineProperty(exports, ..) #222
Conversation
edeustace
changed the title
There was a problem hiding this comment.
LGTM. Thanks for taking the time to put this together, we really appreciate it!
@lukastaegert @NotWoods please have a look
packages/commonjs/src/transform.js
Outdated
| @@ -299,6 +318,10 @@ export function transformCommonjs( | |||
| return; | |||
| } | |||
|
|
|||
| // Is this a call to Object.defineProperty(exports, ...)? | |||
| const def = defineProperty(node, 'exports'); | |||
There was a problem hiding this comment.
I would rename the function to isDefinePropertyCall. Then the comment is also not needed. The return value could be propertyName, then I do not have to check the function definition what it means.
There was a problem hiding this comment.
done @lukastaegert 👍
There was a problem hiding this comment.
actually on reflection I propose it be called getDefinePropertyCallName, the java programmer in me is complaining that an isX function should only return a boolean. See latest commit. That said - happy to rollback if you prefer isDefinePropertyCall.
There was a problem hiding this comment.
Why not return a boolean?
There was a problem hiding this comment.
We could, but we’d also need to get the property name to add to namedExports?
There was a problem hiding this comment.
You are right, and you definitely got what I meant 😉
|
|
…(exports, ..) (rollup#222) * feat: support Object.defineProperty(exports, ..) * chore: rename defineProperty -> isDefinePropertyCall * chore: rename isDefinePropertyCall -> getDefinePropertyCallName
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This pr adds exports that have been defined using the
Object.defineProperty(exports, ...)technique. I was running into an issue with a 3rd party lib (@material-ui/[email protected]) where it was using this technique to define the exports. Rollup was falling over with the errorexport 'foo' is not exported by 'path/to/module'.