Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

docs(Examples): use sources to render dedicated examples, separate contexts #644

Merged
merged 7 commits into from
Jan 2, 2019
Merged

docs(Examples): use sources to render dedicated examples, separate contexts #644

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bcb97e1
docs(Examples): separate contexts, use sources to render dedicated ex…
layershifter Dec 19, 2018
c93098a
Merge branch 'master' into docs/use-source-render
layershifter Dec 19, 2018
19aada0
Merge branches 'docs/use-source-render' and 'master' of https://githu…
layershifter Dec 21, 2018
047834f
fixes to review comments
layershifter Jan 2, 2019
3526341
simplify regex
layershifter Jan 2, 2019
994b6e9
Merge branches 'docs/use-source-render' and 'master' of https://githu…
layershifter Jan 2, 2019
93e1bc0
restore changes
layershifter Jan 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/src/components/ComponentDoc/ComponentExample/SourceCodeManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { exampleSourcesContext } from '../../../utils'

interface SourceCodeData {
path: string
code: string
Expand Down Expand Up @@ -84,9 +86,9 @@ class SourceCodeManager {

private safeRequire = (path: string): string | undefined => {
try {
const filename = `${path.replace(/^components\//, '')}.source.json`
const filename = `${path.replace(/^components\//, './')}.source.json`

return require(`!docs/src/exampleSources/${filename}`).js
return exampleSourcesContext(filename).js
} catch (e) {
return undefined
}
Expand Down
24 changes: 12 additions & 12 deletions docs/src/components/ComponentDoc/ComponentExamples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as _ from 'lodash'
import * as PropTypes from 'prop-types'
import * as React from 'react'

import { exampleContext } from 'docs/src/utils'
import { exampleGroupsContext, exampleSourcesContext } from 'docs/src/utils'
import { Grid, List } from 'semantic-ui-react'
import { examplePathPatterns } from './ComponentExample'
import ContributionPrompt from './ContributionPrompt'
Expand Down Expand Up @@ -30,23 +30,22 @@ export default class ComponentExamples extends React.Component<ComponentExamples
*/
private renderExamples = (): JSX.Element | null => {
const { displayName } = this.props
const allPaths = exampleContext.keys()

// rule #1
const indexPath = _.find(allPaths, path =>
const indexPath = _.find(exampleGroupsContext.keys(), path =>
new RegExp(`\/${displayName}\/index\.tsx$`).test(path),
)
if (!indexPath) {
return null
}

const ExamplesElement = React.createElement(exampleContext(indexPath).default) as any
const ExamplesElement = React.createElement(exampleGroupsContext(indexPath).default) as any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code which also suggests that exampleIndexContext would aid readability here. With current name used it was necessary for me to verify that all the index files are bundled by exampleGroupsContext

if (!ExamplesElement) {
return null
}

// rules #2 and #3
const missingPaths = this.testExamplesStructure(displayName, allPaths)
const missingPaths = this.testExamplesStructure(displayName, exampleSourcesContext.keys())
return missingPaths && missingPaths.length ? (
<div>
{this.renderMissingShorthandExamples(missingPaths)} {ExamplesElement}
Expand Down Expand Up @@ -82,23 +81,24 @@ export default class ComponentExamples extends React.Component<ComponentExamples
)

private testExamplesStructure(displayName: string, allPaths: string[]): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the return value of this method, it seems to be a good idea to rename it to something that is more descriptive, like getMissingExamplePaths - as otherwise it is completely unclear what this method is supposed to return

const examplesPattern = `\.\/\\w*\/${displayName}[\\w\/]*\/\\w+Example`
const allExamplesRegExp = new RegExp(`${examplesPattern}[\\w\.]*\.tsx$`)
const examplesPattern = `\.\/${displayName}[\\w\/]*\/\\w+Example`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that there is a problem with the regular expression used here - specifically, with the following part of it:

[\w\/]

Here it seems that the intent is to match either word character or slash, but the problem is that idle slash at start makes this regex to match w character literally.

This one will work correctly:

[\w\\]

The same problem apparently happens to the rest of the expression, where double slash is used here: \\w+Example. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified it to:

const examplesPattern = `\./${displayName}/[\\w/]+Example`

examplesPattern is not regex expression, it's a regular string. I'm not sure that we need there regular expressions at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much simpler to analyze, thanks!

still there is a problem, unfortunately - and this comes from the fact that slashes are not escaped in the string expression. This is what string expression will be evaluated to: ./displayName/[w/]+Example (with displayName='displayName')

And this expression creates several problems:

  • each of the path delimiter slashes should be escaped
  • dot at the start will match any character (is it our intent, or we would like to match dot literally? could we narrow down the set of characters to match, if so?)

I would see the following string should be used instead:

// here I suppose that intent is to match dot character literally, 
// please, make any necessary corrections if it is not
const examplesPattern = `[.][/]${displayName}[/][\w/]+Example`

Please, just verify once again this regular expression before merging these changes. Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, merged before this comment 😿

escaped in the string expression

They will be escaped in new Regexp():

const displayName = 'Label'
const r = new RegExp(`\./${displayName}/[\\w/]+Example`)

r.toString() // "/.\/Label\/[\w\/]+Example/"
r.test('./Label/Types/LabelExample') // true


const [normalExtension, shorthandExtension] = [
examplePathPatterns.normal,
examplePathPatterns.shorthand,
].map(pattern => `${pattern}.tsx`)
].map(pattern => `${pattern}.source.json`)

const [normalRegExp, shorthandRegExp] = [normalExtension, shorthandExtension].map(
extension => new RegExp(`${examplesPattern}\\w*${extension}$`),
)

const allExamplesPaths = allPaths.filter(path => allExamplesRegExp.test(path))
const expectedShorthandExamples = allExamplesPaths
const expectedShorthandExamples = allPaths
.filter(path => normalRegExp.test(path))
.map(path => path.replace(normalExtension, shorthandExtension))
const actualShorthandExamples = allExamplesPaths.filter(path => shorthandRegExp.test(path))
const actualShorthandExamples = allPaths.filter(path => shorthandRegExp.test(path))

return _.difference(expectedShorthandExamples, actualShorthandExamples)
return _.difference(expectedShorthandExamples, actualShorthandExamples).map(exampleFile =>
exampleFile.replace(/\.source\.json$/, '.tsx'),
)
}
}
32 changes: 25 additions & 7 deletions docs/src/components/ExternalExampleLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import * as _ from 'lodash/fp'
import * as PropTypes from 'prop-types'
import * as React from 'react'
import SourceRender from 'react-source-render'

import { exampleContext, exampleKebabNameToFilename, parseExamplePath } from 'docs/src/utils'
import { ExampleSource } from 'docs/src/types'
import { exampleSourcesContext, exampleKebabNameToFilename, parseExamplePath } from 'docs/src/utils'
import PageNotFound from '../views/PageNotFound'
import { babelConfig, importResolver } from './Playground/renderConfig'

const examplePaths = exampleContext.keys()
const examplePaths = exampleSourcesContext.keys()

const ExternalExampleLayout: any = props => {
const { exampleName } = props.match.params
Expand All @@ -17,11 +20,26 @@ const ExternalExampleLayout: any = props => {
}, examplePaths)

if (!examplePath) return <PageNotFound />

const ExampleComponent = exampleContext(examplePath).default
if (!ExampleComponent) return <PageNotFound />

return <ExampleComponent />
const exampleSource: ExampleSource = exampleSourcesContext(examplePath)

return (
<SourceRender
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not sure why we are relying on the React Context functionality to render source code - but this is definitely not the point of this PR, we could address this moment later (if necessary)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking for a better solution there.

babelConfig={babelConfig}
source={exampleSource.js}
renderHtml={false}
resolver={importResolver}
>
<SourceRender.Consumer>
{({ element, error }) => (
<>
{element}
{/* This block allows to see issues with examples as visual regressions. */}
{error && <div style={{ fontSize: '5rem', color: 'red' }}>{error.toString()}</div>}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little clumsy, but it will show us broken examples as visual regressions.

</>
)}
</SourceRender.Consumer>
</SourceRender>
)
}

ExternalExampleLayout.propTypes = {
Expand Down
11 changes: 0 additions & 11 deletions docs/src/utils/exampleContext.ts
View file
Open in desktop

This file was deleted.

13 changes: 13 additions & 0 deletions docs/src/utils/exampleContexts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Get the Webpack Context for doc site example groups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: suggest to omit 'Get' verb as these exports just provide corresponding context modules, while 'get' action happens on the client side.

Also, while 'groups' is also correct, would suggest to rename this context to exampleIndexContext - it will be easier for the reader to immediately infer the files implied by this context, without any additional necessity to remind terminology that is used by docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, renamed to exampleIndexContext and removed the get verb 👍

*/
export const exampleGroupsContext = require.context('docs/src/examples/', true, /index.tsx$/)

/**
* Get the Webpack Context for doc site example sources.
*/
export const exampleSourcesContext = require.context(
'docs/src/exampleSources/',
true,
/.source.json$/,
)
15 changes: 0 additions & 15 deletions docs/src/utils/exampleKebabNameToFilename.ts
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions docs/src/utils/exampleKebabNameToSourceFilename.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as _ from 'lodash/fp'

/**
* Converts kebab-cased-example-name back into the original filename.
* @param {string} exampleKebabName
*/
const exampleKebabNameToSourceFilename = exampleKebabName => {
// button-example => ButtonExample.source.json
// button-example-shorthand => ButtonExample.shorthand.source.json
return `${_.startCase(exampleKebabName)
.replace(/ /g, '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can be a bit more expressive here. The only source of the whitespace could be either leading or trailing ones of the initial kebab-cased name. So, we might eliminate them before transform will happen, like that:

const trimmedExampleKebabName = exampleKebabName.trim()

return `${_.startCase(trimmedExampleKebabName) ... }...`

While this makes code more efficient, I would put efficiency as the least worthy benefit in this case - the primary goal of this move is rather to improve explicitness of where the spaces (that regex aims to eliminate) might happen from.

Copy link
Member Author

@layershifter layershifter Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed solution is not correct because _.startCase() will create spaces:

_.startCase('foo-bar')
// => 'Foo Bar'

.replace(/Shorthand$/, '.shorthand')}.source.json`
}

export default exampleKebabNameToSourceFilename
4 changes: 2 additions & 2 deletions docs/src/utils/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
export { default as componentInfoContext } from './componentInfoContext'
export { default as componentInfoShape } from './componentInfoShape'
export { default as exampleContext } from './exampleContext'
export { default as exampleKebabNameToFilename } from './exampleKebabNameToFilename'
export { exampleGroupsContext, exampleSourcesContext } from './exampleContexts'
export { default as exampleKebabNameToFilename } from './exampleKebabNameToSourceFilename'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is not hard to do (which it seems not), I would suggest to rename the reexported entry so it will match the name of the file it is imported from

export { default as examplePathToHash } from './examplePathToHash'
export { default as getComponentGroup } from './getComponentGroup'
export { default as getComponentPathname } from './getComponentPathname'
Expand Down