Named-exports extras - Default export function with named exports #2186
Conversation
dist/extras/named-exports.js
Outdated
| @@ -3,7 +3,7 @@ | |||
| */ | |||
| (function (global) { | |||
| var systemJSPrototype = global.System.constructor.prototype; | |||
|
|
|||
There was a problem hiding this comment.
dist is in source control, but we only change it during publishes. We're considering removing it from source control in v7 to make it easier for contributors to send in PRs. But for now, could you remove your changes to dist?
git checkout master -- dist
src/extras/named-exports.js
Outdated
| for (var exportName in defaultExport) { | ||
| // default is not a named export | ||
| if (exportName !== 'default') { | ||
| if (defaultExport.hasOwnProperty(exportName) && exportName !== 'default') { |
There was a problem hiding this comment.
I'm wondering if there is ever a case where the defaultExport does not have a hasOwnProperty function. The defaultExport can have any type of value. I've checked and numbers, strings, symbols, functions, objects, and arrays and they do have it. However, it's possible that someone could modify the prototype of the value such that it doesn't have a default export. An example is that ES module "namespace" objects do not have the object prototype, and are created with Object.create(null).
A tangential example where the object prototype is removed from some things - #2147. That particular example doesn't demonstrate a specific case where the code here would break things, but just shows that there are situations where values do not have the Object prototype.
In such cases where hasOwnProperty does not exist, this code will throw an error. Perhaps the safer implementation would be this?
| if (defaultExport.hasOwnProperty(exportName) && exportName !== 'default') { | |
| if (Object.prototype.hasOwnProperty.call(defaultExport, exportName) && exportName !== 'default') { |
There was a problem hiding this comment.
Yes, Object.hasOwnProperty is preferred for this reason.
| if (typeof define === 'function' && define.amd) { | ||
| define(['./amd-dep.js'], factory); | ||
| } else { | ||
| throw 'Test suit only supports amd' |
There was a problem hiding this comment.
In this project, we usually use semicolons:
| throw 'Test suit only supports amd' | |
| throw 'Test suit only supports amd'; |
| if (typeof define === 'function' && define.amd) { | ||
| define(['./amd-dep.js'], factory); | ||
| } else { | ||
| throw 'Test suit only supports amd' |
There was a problem hiding this comment.
| throw 'Test suit only supports amd' | |
| throw 'Test suit only supports amd'; |
There was a problem hiding this comment.
Thanks @Sauloxd for carrying this one through, it's a big help.
|
Thanks for reviewing guys! I pushed a new version with your suggestions. Updated:
|
Related issue: #2184
This enhances the
extras/named-exports.jsto allow enumerable keys from default exported functions to be handled as named exports (previous behaviour was to select by object type).I'm not sure if I should add an UMD bundle type fixture (as they seem kinda redundant with AMD fixtures), but it doest feels closer to the problem described in the issue. I can change that if it's better!
With the previous build, only the test regarding
named exports + default object exportpassed (as it was the previous behaviour) and thenamed exports + default function exportfailsWith the new build, all tests passes.
Obs
I have no idea why
dist/system-node.cjsfile has extra code included (I built it with:yarn install+yarn build). Maybe it's due to my node-version/yarn-version being different or something like that?