-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add limited support for named exports. Add docs relating to this support. #825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
59b1a71
69563d6
9937186
5f369bd
d3c309d
dcdee30
5e987e6
a3fde51
9e07efc
90b998f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import getComponent from '../getComponent'; | ||
|
||
describe('getComponent', () => { | ||
describe('if there is a default export in the module', () => { | ||
it('should return that', () => { | ||
const module = { default: 'useMe' }; | ||
const actual = getComponent(module); | ||
expect(actual).toBe(module.default); | ||
}); | ||
}); | ||
|
||
describe('if it is a CommonJS module and exports a function', () => { | ||
it('should return the module', () => { | ||
const testCases = [() => {}, function() {}, class Class {}]; | ||
testCases.forEach(testCase => { | ||
const actual = getComponent(testCase); | ||
expect(actual).toBe(testCase); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('if there is only one named export in the module', () => { | ||
it('should return that', () => { | ||
const module = { oneNamedExport: 'isLonely' }; | ||
const actual = getComponent(module); | ||
expect(actual).toBe(module.oneNamedExport); | ||
}); | ||
}); | ||
|
||
describe('if there is a named export whose name matches the name argument', () => { | ||
it('should return that', () => { | ||
const name = 'Component'; | ||
const module = { [name]: 'isNamed', OtherComponent: 'isAlsoNamed' }; | ||
const actual = getComponent(module, name); | ||
expect(actual).toBe(module[name]); | ||
}); | ||
}); | ||
|
||
describe('if there is more than one named export and no matching name', () => { | ||
it('should fall back on returning the module as a whole', () => { | ||
const name = 'Component'; | ||
const module = { RandomName: 'isNamed', confusingExport: 123 }; | ||
const actual = getComponent(module, name); | ||
expect(actual).toBe(module); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,21 @@ | ||
import globalizeComponent from '../globalizeComponent'; | ||
|
||
afterEach(() => { | ||
delete global.Foo; | ||
}); | ||
const component = { module: 'someModule', name: 'SomeName' }; | ||
|
||
describe('globalizeComponent', () => { | ||
it('should set component’s module as a global variable', () => { | ||
const globalsCount = Object.keys(global).length; | ||
globalizeComponent({ | ||
name: 'Foo', | ||
props: {}, | ||
module: 13, | ||
}); | ||
expect(Object.keys(global).length).toBe(globalsCount + 1); | ||
expect(global.Foo).toBe(13); | ||
afterEach(() => { | ||
delete global[component.name]; | ||
}); | ||
|
||
it('should not add anything as a global variable if there is no component name', () => { | ||
expect(global[component.name]).toBeUndefined(); | ||
globalizeComponent({}); | ||
expect(global[component.name]).toBeUndefined(); | ||
}); | ||
|
||
it('should set the return value of getComponent as a global variable', () => { | ||
expect(global[component.name]).toBeUndefined(); | ||
globalizeComponent(component); | ||
expect(global[component.name]).toBe(component.module); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/** | ||
* Given a component module and a name, | ||
* return the appropriate export. | ||
* See /docs/Components.md | ||
* | ||
* @param {object} module | ||
* @param {string} name | ||
* @return {function|object} | ||
*/ | ||
export default function getComponent(module, name) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess your understanding of this code is very good now, could you please add comments to each variation? Preferably with code examples. This is one of the most obscure parts of the code base ;-/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Please add a blank line before each comment to make it easier to read. |
||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to leave this little 'comment' in here to stop Prettier complaining about the blank line... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wasn't clear enough: blank lines to separate code “paragraphs”, not just empty line before each comment ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you mean changing this
to this
Or something else? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly! Just to separate different sections of code. |
||
// If the module defines a default export, return that | ||
// e.g. | ||
// | ||
// ``` | ||
// export default function Component() { ... } | ||
// ``` | ||
// | ||
if (module.default) { | ||
return module.default; | ||
} | ||
|
||
// If it is a CommonJS module which exports a function, return that | ||
// e.g. | ||
// | ||
// ``` | ||
// function Component() { ... } | ||
// module.exports = Component; | ||
// ``` | ||
// | ||
if (!module.__esModule && typeof module === 'function') { | ||
return module; | ||
} | ||
const moduleKeys = Object.keys(module); | ||
|
||
// If the module exports just one named export, return that | ||
// e.g. | ||
// | ||
// ``` | ||
// export function Component() { ... } | ||
// ``` | ||
// | ||
if (moduleKeys.length === 1) { | ||
return module[moduleKeys[0]]; | ||
} | ||
|
||
// If the module exports a named export with the same name as the | ||
// understood Component identifier, return that | ||
// e.g. | ||
// | ||
// ``` | ||
// // /component.js | ||
// export function someUtil() { ... } | ||
// export Component() { ... } // if identifier is Component, return this named export | ||
// ``` | ||
// | ||
// Else return the module itself | ||
// | ||
return module[name] || module; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to make this section shorted and easier to scan. Maybe use one big table instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resorting to HTML table format here to play nicely with the code blocks. It makes this file a little ugly but the output is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be simpler:
Also you can remove the last column and use the same component name everywhere, and merge displayName and fallback columns since they are mutually exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah nice. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, it's like 10 times smaller now ;-)