Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Move component-index generation to scaffold-config package #21090

Merged
merged 4 commits into from
Apr 20, 2022
Merged

chore: Move component-index generation to scaffold-config package #21090

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ccc895
chore: Move component-index generation to scaffold-config package
BlueWinds Apr 14, 2022
327bafc
Merge remote-tracking branch 'origin/10.0-release' into UNIFY-1433-re…
BlueWinds Apr 19, 2022
9d4e27f
Review fixes
BlueWinds Apr 19, 2022
01f8928
Fix review comments for realzies this time
BlueWinds Apr 19, 2022
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
62 changes: 4 additions & 58 deletions packages/data-context/src/actions/WizardActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ const debug = Debug('cypress:data-context:wizard-actions')

import type { DataContext } from '..'

interface WizardGetCodeComponent {
chosenLanguage: 'js' | 'ts'
chosenFramework: typeof WIZARD_FRAMEWORKS[number]
}

export class WizardActions {
constructor (private ctx: DataContext) {}

Expand Down Expand Up @@ -200,10 +195,7 @@ export class WizardActions {
this.scaffoldFixtures(),
this.scaffoldSupport('component', chosenLanguage),
this.scaffoldSupport('commands', chosenLanguage),
this.getComponentIndexHtml({
chosenFramework,
chosenLanguage,
}),
this.scaffoldComponentIndexHtml(chosenFramework),
])
}

Expand Down Expand Up @@ -310,64 +302,18 @@ export class WizardActions {
return codeBlocks.join('\n')
}

private async getComponentIndexHtml (opts: WizardGetCodeComponent): Promise<NexusGenObjects['ScaffoldedFile']> {
const [storybookInfo] = await Promise.all([
this.ctx.storybook.loadStorybookInfo(),
this.ensureDir('component'),
])
const framework = opts.chosenFramework.type
let headModifier = ''
let bodyModifier = ''

if (framework === 'nextjs') {
headModifier += '<div id="__next_css__DO_NOT_USE__"></div>'
}

const previewHead = storybookInfo?.files.find(({ name }) => name === 'preview-head.html')

if (previewHead) {
headModifier += previewHead.content
}

const previewBody = storybookInfo?.files.find(({ name }) => name === 'preview-body.html')

if (previewBody) {
headModifier += previewBody.content
}

const template = this.getComponentTemplate({
headModifier,
bodyModifier,
})
private async scaffoldComponentIndexHtml (chosenFramework: typeof WIZARD_FRAMEWORKS[number]): Promise<NexusGenObjects['ScaffoldedFile']> {
await this.ensureDir('component')

const componentIndexHtmlPath = path.join(this.projectRoot, 'cypress', 'support', 'component-index.html')

return this.scaffoldFile(
componentIndexHtmlPath,
template,
chosenFramework.componentIndexHtml(),
'The HTML used as the wrapper for all component tests',
)
}

private getComponentTemplate = (opts: { headModifier: string, bodyModifier: string }) => {
// TODO: Properly indent additions and strip newline if none
return dedent`
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>
${opts.headModifier}
</head>
<body>
${opts.bodyModifier}
<div data-cy-root></div>
</body>
</html>`
}

private async scaffoldFile (filePath: string, contents: string, description: string): Promise<NexusGenObjects['ScaffoldedFile']> {
try {
debug('scaffoldFile: start %s', filePath)
Expand Down
28 changes: 28 additions & 0 deletions packages/scaffold-config/src/component-index-template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import dedent from 'dedent'

const componentIndexHtmlGenerator = (headModifier: string = '', bodyModifier: string = '') => {
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
return () => {
const template = dedent`
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>
${headModifier}
</head>
<body>
${bodyModifier}
<div data-cy-root></div>
</body>
</html>
`

// If the framework returns an empty string for either of the modifiers,
// strip out the empty lines
return template.replace(/\n {4}\n/g, '\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should be using https://nodejs.org/api/os.html#oseol

os.EOL // =>
    \n on POSIX
    \r\n on Windows

I think this would fail on windows?

Copy link
Contributor Author

@BlueWinds BlueWinds Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hm. I don't think this is the right place for that - if we scaffold files using OS EOL separators, then that would be done in the scaffoldFile method in data-context/src/actions/WizardActions.ts. Then it would be in one place, applying to all files we scaffold to disk.

I don't think we should do that, though. Several other methods in that same file (scaffoldFixtures and scaffoldConfig for example) use \n as well. It'd be a bit of a change and would probably want its own ticket.

}
}

export default componentIndexHtmlGenerator
9 changes: 9 additions & 0 deletions packages/scaffold-config/src/frameworks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import path from 'path'
import dedent from 'dedent'
import fs from 'fs-extra'
import * as dependencies from './dependencies'
import componentIndexHtmlGenerator from './component-index-template'
import { defineConfigAvailable } from '@packages/data-context/src/sources/migration/codegen'
import semver from 'semver'
import resolveFrom from 'resolve-from'
Expand Down Expand Up @@ -122,6 +123,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'react',
glob: '*.{js,jsx,tsx}',
mountModule: 'cypress/react',
componentIndexHtml: componentIndexHtmlGenerator(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach. It seems like there's duplication (there is) but I think it's actually somewhat ideal. That said, if you forgot to add componentIndexHtml, the only way we'd know is via a type error.

One thing we could do is add a test that loops each of these, asserting the output, like

for (const framework of WIZARD_FRAMEWORKS) {
  const actual = framework.componentIndexHtmlGenerator()
  if (framework === 'nextjs') {
    // specific assertion
  } else {
    expect(actual).to.eq(<generic_template>)
  }
}

Do you think this would be useful to have, or the types are enough to keep us safe and confident? I could go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that relying on types is fine - if they're not here to catch issues like this, then why do we even have them? It's also implicitly covered by the system tests (though I wouldn't want to rely on them to cover this feature, they are a backstop against the types as well).

},
{
type: 'vueclivue2',
Expand All @@ -142,6 +144,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'vue',
glob: '*.vue',
mountModule: 'cypress/vue2',
componentIndexHtml: componentIndexHtmlGenerator(),
},
{
type: 'vueclivue3',
Expand All @@ -162,6 +165,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'vue',
glob: '*.vue',
mountModule: 'cypress/vue',
componentIndexHtml: componentIndexHtmlGenerator(),
},
{
type: 'nextjs',
Expand All @@ -181,6 +185,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'react',
glob: '*.{js,jsx,tsx}',
mountModule: 'cypress/react',
componentIndexHtml: componentIndexHtmlGenerator(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
componentIndexHtml: componentIndexHtmlGenerator(),
componentIndexHtml: componentIndexHtmlGenerator('<div id="__next_css__DO_NOT_USE__"></div>')

We have the react and next templates swapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Fixed in upcoming commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was added to nuxt instead next!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for realzies this time! ^^;;

},
{
type: 'nuxtjs',
Expand All @@ -200,6 +205,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'vue',
glob: '*.vue',
mountModule: 'cypress/vue2',
componentIndexHtml: componentIndexHtmlGenerator(),
},
{
type: 'vue2',
Expand All @@ -219,6 +225,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'vue',
glob: '*.vue',
mountModule: 'cypress/vue2',
componentIndexHtml: componentIndexHtmlGenerator(),
},
{
type: 'vue3',
Expand All @@ -238,6 +245,7 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'vue',
glob: '*.vue',
mountModule: 'cypress/vue',
componentIndexHtml: componentIndexHtmlGenerator(),
},
{
type: 'react',
Expand All @@ -257,5 +265,6 @@ export const WIZARD_FRAMEWORKS = [
codeGenFramework: 'react',
glob: '*.{js,jsx,tsx}',
mountModule: 'cypress/react',
componentIndexHtml: componentIndexHtmlGenerator('<div id="__next_css__DO_NOT_USE__"></div>'),
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
},
] as const
47 changes: 47 additions & 0 deletions packages/scaffold-config/test/unit/component-index-template.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { expect } from 'chai'
import dedent from 'dedent'
import componentIndexHtmlGenerator from '../../src/component-index-template'

describe('componentIndexHtmlGenerator', () => {
it('strips spaces and newlines appropriately', () => {
const generator = componentIndexHtmlGenerator()

const expected = dedent`
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>
</head>
<body>
<div data-cy-root></div>
</body>
</html>`

expect(generator()).to.eq(expected)
})

it('handles header and body modifiers', () => {
const generator = componentIndexHtmlGenerator('foobar', 'baz')

const expected = dedent`
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>
foobar
</head>
<body>
baz
<div data-cy-root></div>
</body>
</html>`

expect(generator()).to.eq(expected)
})
})
4 changes: 1 addition & 3 deletions ...ne/expected-cypress-js-component-create-react-app-v5/cypress/support/component-index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>

</head>
<body>

<div data-cy-root></div>
</body>
</html>
</html>
4 changes: 1 addition & 3 deletions ...ne/expected-cypress-ts-component-create-react-app-v5/cypress/support/component-index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>

</head>
<body>

<div data-cy-root></div>
</body>
</html>
</html>