Fix unused import in generator cell test template

[Philzen]

Closes #5060

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	13d74d3
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62f112aca5ba2100079e1d33

[dac09]

Hi again @Philzen!

This import is something I've been aware of, but I wonder if its not worth removing this import? I would instead add the unused screen imports to the page and component generators probably.

Generators often produce code that doesn't pass linting, and thats totally okay, if it serves another purpose - in this case, giving people a hint that screen can be imported from the package.

[Philzen]

Generators often produce code that doesn't pass linting

Well, that's actually redwood's very own linting rules 🤷 ... and thus makes my code tab red 😢

and thats totally okay

I beg to differ – personally my level of trust in a framework degrades if it generates code that doesn't pass its own linting rules, that's a contradiction of sorts.

giving people a hint that screen can be imported from the package.

Yes there is a clear, unambiguous hint where screen needs to imported from if required.

redwood/packages/cli/src/commands/generate/cell/__tests__/__snapshots__/cell.test.js.snap

Lines 115 to 119 in 2513813

	// When you're ready to test the actual output of your component render
	// you could test that, for example, certain text is present:
	//
	// 1. import { screen } from '@redwoodjs/testing/web'
	// 2. Add test: expect(screen.getByText('Hello, world')).toBeInTheDocument()

IMHO there is absolutely no advantage to keeping an unused import lying around in generated files – which may or may not ever be used – or is there?

[dthyresson]

@jtoar refresh my memory, was screen added for accessibility needs?

[jtoar]

@dthyresson No I don't think so. Just went through the blame, and it was added quite some time ago: #290. But the template code that uses screen has long since been removed: #1397.

One thing I find odd is that I only see screen in TS projects, not JS projects. transformTSToJS must strip it because TS complains. For that reason and the ones that @Philzen mentioned I'm ok with this change. But @dac09 feel free to overrule me if you still disagree and we can discuss with the team at large.

[jtoar]

@Philzen I've merged main into this branch. I think you've expressed distaste at that before, so sorry that I went ahead and did that! I just want to make sure checks pass on main, and I don' think anymore code changes need to be made.

[Philzen]

@jtoar no worries, as those ugly update merge commits are squashed anyway on merge to main it doesn't matter for the commit history.

It's only a nuisance if there were changes requested and i need to add additional (fixup) commits, b/c with merge strategy my commits are buried deep down in the history, while i am used to see my unmerged work always on top.

So imho we're doing contributors a favor when there are still changes requested to use the rebase button to pull in the recent updates from main:

[jtoar]

Approving just to get this off my "needs review" list, but still open to discuss @dac09 if you disagree with my comment earlier

[dac09]

Hi all, going to close this, as it will be superceded by #6058 - where all tempaltes are now going to use screen (for different reasons).

Thanks again @Philzen !

[Philzen]

Ah i see: https://github.com/redwoodjs/redwood/pull/6058/files#diff-61b70e171dde41fe8e245f38f44c78d8ec3354923c8b8b9a9713d1b1f7db74f8R15

That's cool with me – IMHO this is a clear improvement to the test template. 👍

[Philzen]

Hi all, going to close this, as it will be superceded by #6058 - where all tempaltes are now going to use screen (for different reasons).

Oh, i just realized #6058 has been cancelled / not merged. The successor #5985 (which is still a draft) doesn't seem to fix #5060. @dac09 maybe reopen & merge this PR for the time being?

Then we could close #5060 and move on w/o the generator producing red squiggles in generated tests 😉

[Tobbe]

So we discussed the generated tests on a team meeting.

We decided we're not going to use screen in the generated tests. The reason is given here: #1397

We did not however (iirc) decide if we should still leave the screen import in there or not. I'll reopen this PR for now.

[Tobbe]

We have this comment further down in the file

I know we said on the meeting that we sometimes generate unused code to educate our users on how to use the code, to discover features etc. In these files I think that generated comment is enough to show the users how to write tests. So I think we can remove the unused import. @dac09 What do you think? Is the comment enough?

[Philzen]

We decided we're not going to use screen in the generated tests. The reason is given here: #1397

Oh yeah, that is a darn good reason for not using screen in the test template that @cannikin brought up there, i did not think of that catch 🤦‍♂️, thanks for sharing.

We did not however (iirc) decide if we should still leave the screen import in there or not. I'll reopen this PR for now.

Cheers @Tobbe – just wanted to add that @jtoar confirmed that this only seems to apply to typescript generated tests, not js. So should the team decide to keep it in this should be at least harmonized with the JS behaviour.

I'd like to add that i strongly feel we should strive to ensure the generator never produces code that does not follow the linting rules – as it produces red squiggles in the IDE. Such a behaviour adds uncertainty to the DX (i'm speaking from my own experience and other Redwood beginners have also expressed that) and what's worse, it could degrade the perceived quality / stability of the (awesome!) project and its (awesome!) generators.

[dac09]

I'd like to add that i strongly feel we should strive to ensure the generator never produces code that does not follow the linting rules – as it produces red squiggles in the IDE

I understand Philzen - I don't think we'll ever really see eye to eye on this in all honesty - but I'll try and explain my position anyway.

1. Generators do not produce code that you just run and then deploy.
This is not the purpose of generators. Its to produce boilerplate. We WANT people to look at the code, and change it for their project.

2. Its impossible to make sure generators work for all projects, across all prisma schemas, across all auth providers, across all deployment targets
Your project is different and there's absolutely no way of forseeing every possible possible permutation a Redwood project can take. We could add eslint ignore and tsignores everywhere so all the generated code is always green - but when you actually have a bug in your code, for example, because you decided to change a property name, none of the checks will catch it because we're telling all the tooling to ignore it. The downside of this is far more severe than the upside.

Linting errors are just stylistic opinions - which ofcourse in your project you decide how severe or lenient you want to be.

3. Unused variables in some templates
If we add eslint ignores, a large part of generator templates will be eslint comments - this promoting bad practice for the sake of making things look good. If we just remove those variables we take away some of the "implicit learning" you get from just reading the code - which IMO is far more valuable (and saves you much more time).

[dac09]

Just needs a test-project rebuild.

[dac09]

So the PR got a little bit stale - lots of merge conflicts. I've reapplied your changes @Philzen and regenerated the project in #5827 - wlil merge it in as soon as its green.

Thanks for taking the time!

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 13d74d3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 4 targets

• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run build
• cli run build

---

Sent with 💌 from NxCloud.

[Philzen]

@dac09 no need for conflict.

I simply did an interactive rebase on main, and added a commit to update the fixtures. I believe you can simply merge this one.

[Philzen]

@dac09 @Tobbe this of course still fails because one of the jest snapshots doesn't match anymore. Can you throw me the command how to update the snapshots?

[jtoar]

From the root of your local copy of the RW framework, it should be:

yarn workspace @redwoodjs/cli run test -u

[Philzen]

From the root of your local copy of the RW framework, it should be:

yarn workspace @redwoodjs/cli run test -u

Cheers @jtoar – So since yesterday i learned about fixtures and snapshot by nagging you guys from the core team ;)

I briefly checked https://redwoodjs.com/docs/contributing-walkthrough and https://redwoodjs.com/docs/contributing for "snapshot" but couldn't find any info.

Should we maybe take a note / issue to add this info to the PR process documentation? On the other hand, this has the potential to scare newbies away, but i guess it's "no CI pain, no gain" 😉

[Tobbe]

This is what I usually do when I need to update the snapshots (I use auth in the example, but obviously adjust to your needs)

cd packages/auth
yarn jest -u

[Philzen]

@dac09

1. Generators do not produce code that you just run and then deploy.
Hell no (and i never assumed that) – nobody in his right mind would or should do that except for Chuck Norris 😉

Your project is different and there's absolutely no way of forseeing every possible possible permutation a Redwood project can take.

Sure, and i don't believe we have to. Currently, all of these issues i worked on were out-of-the-box, mint-configuration eslint generator violations. Talking about my project – no it's actually not (yet) that different – we're using the default linting rules shipped in the ESLint-package (although we'll soon improve upon them and i am also planning to suggest some for Redwood 🧐). As soon as a user goes down the road to customize them, they will be more advanced, and expect one or the other red squiggle, as they will be knowing what they are doing. But then they're out of the woods and of course we don't need to cater for their customized environments.

We could add eslint ignore and tsignores everywhere so all the generated code is always green

1. Again, i did not advocate for that as a solution for everything – my intent is simply to have generators produce code that passes our own linting rules, not more, not less. I believe it is an important marketing factor to build trust in the project. Newbies will easily think that they did something wrong (I have experienced that insecurity in the beginning, and other people have expresses that too), or worse, they'll the framework has a bug, respectively doesn't take it's own linting rules seriously despite imposing them on the users. An #opinionatedFramework takes away freedom, but within those boundaries that it imposes, it should strive to be as flawless as possible.
2. but actually, as you mentioned it, the generators code base is already sprinkled with dozens of those ignore rules (just do a full-text-search for no-unused-vars) – which i believe it is is OK in some cases to be able to illustrate something to our users, w/o confusing them with red squiggles. At the end of the day: it's a trade-off.

I simply believe it would be a great win if we could just add yarn rw lint to the CI pipeline and keep our out-of-the-box code validated against our own ESLint rules all the time, as suggested before.