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
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Looks good to me too, just a minor naming suggestion.
| } | ||
|
|
||
| // 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.
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
4 participants
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/core@3.9.4) 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'.