chore: Move component-index generation to scaffold-config package #21090
Conversation
BlueWinds
requested review from
jennifer-shehane
and removed request for
a team
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -122,6 +123,7 @@ export const WIZARD_FRAMEWORKS = [ | |||
| codeGenFramework: 'react', | |||
| glob: '*.{js,jsx,tsx}', | |||
| mountModule: 'cypress/react', | |||
| componentIndexHtml: componentIndexHtmlGenerator(), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
|
||
| // 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') |
There was a problem hiding this comment.
I wonder if we should be using https://nodejs.org/api/os.html#oseol
os.EOL // =>
\n on POSIX
\r\n on WindowsI think this would fail on windows?
There was a problem hiding this comment.
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.
| @@ -181,6 +185,7 @@ export const WIZARD_FRAMEWORKS = [ | |||
| codeGenFramework: 'react', | |||
| glob: '*.{js,jsx,tsx}', | |||
| mountModule: 'cypress/react', | |||
| componentIndexHtml: componentIndexHtmlGenerator(), | |||
There was a problem hiding this comment.
| componentIndexHtml: componentIndexHtmlGenerator(), | |
| componentIndexHtml: componentIndexHtmlGenerator('<div id="__next_css__DO_NOT_USE__"></div>') |
We have the react and next templates swapped.
There was a problem hiding this comment.
Whoops. Fixed in upcoming commit.
There was a problem hiding this comment.
This was added to nuxt instead next!!
There was a problem hiding this comment.
Fix for realzies this time! ^^;;
…factor-component-index.html
…e-config * 10.0-release: chore: Move component-index generation to scaffold-config package (#21090) fix: label text should be clickable to toggle snapshot highlight (#21122) feat: add next preset to webpack-dev-server-fresh (#21069) chore: add dev-servers as deps to server to be included in the binary (#21142) fix: do not highlight preExtension if selected option is renameFolder (#21121) fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063) feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648) test: add windows-test-kitchensink job (#20847) fix: aut centering and height calculation (#21019) chore: fix tests that fail on windows (#21055) fix: running a new test after already having run tests (#21087) fix: ct project setup framework keys for next and nuxt (#21116) fix: remove MountReturn from scaffolded ct support file (#21119) fix: do not scaffold fixtures if folder exist (#21078) fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104) chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083) fix: types for Cypress.Commands.add (#20376) (#20377)
User facing changelog
None. Internal change only.
Additional details
We wanted to move the template for component-index.html to live with the rest of the framework-specific code. This is general code cleanup related to the ct-archecture changes.
How has the user experience changed?
Only effect is a small fix to remove a couple of blank lines from the generated
component-index.hmtl.PR Tasks
cypress-documentation?type definitions?cypress.schema.json?