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.

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

Adds deprecation notice if using Vite but have .js files when you should have .jsx #8712

Merged
merged 13 commits into from
Jun 27, 2023

Conversation

cannikin
Copy link
Member

@cannikin cannikin commented Jun 23, 2023

image

Does a simple search for App.js, Routes.js, pages and components that have a .js extension. An app will have legitimate JS-only files with a .js extension so we don't search ALL of web/src. We also don't search the contents of the files for JSX tags, which may be overkill, and may slow down startup depending on how many files there are.

If the dev is seeing this error erroneously (because they have real JS-only files in web/src/pages or web/src/components it can be disabled with a REDWOOD_DISABLE_JS_DEPRECATION_NOTICE ENV var.

Questions

  • Not sure if the text is really accurate: will .js files actually not work at all in 7.0.0, or will you just not have an optimal experience (like fast reload)?
  • If .js files will technically still work, maybe this notice should be reworded to something about "recommend" and "optimal experience" and actually keep it forever, not remove it as of 7.0?

@cannikin cannikin added the release:chore This PR is a chore (means nothing for users) label Jun 23, 2023
@cannikin cannikin added this to the v6.0.0 milestone Jun 23, 2023
@cannikin cannikin requested a review from dac09 June 23, 2023 20:17
@cannikin
Copy link
Member Author

cannikin commented Jun 23, 2023

@dac09 We could have this warning run during build as well, but that's most likely happening on a remove server and you won't actually see the output...

Unless we process.exit(1) and not even let the build complete? Maybe we save that for 7.0 if/when we make .jsx required.

@dac09
Copy link
Collaborator

dac09 commented Jun 26, 2023

Thanks Rob, leaving a couple of comments :)

@dac09
Copy link
Collaborator

dac09 commented Jun 26, 2023

@dac09 We could have this warning run during build as well, but that's most likely happening on a remove server and you won't actually see the output...

Unless we process.exit(1) and not even let the build complete? Maybe we save that for 7.0 if/when we make .jsx required.

Yeah I think on dev is enough. When we remove JSX-In-JS, It'll fail automatically (I think).

@cannikin
Copy link
Member Author

cannikin commented Jun 26, 2023

Suggestions added, here's the new output:

image

Just need to coordinate so that either @jtoar names the codemod like that, or I update the output to match the real name.

@dac09 dac09 enabled auto-merge (squash) June 27, 2023 05:15
@dac09
Copy link
Collaborator

dac09 commented Jun 27, 2023

or I update the output to match the real name.

Updated to match the codemod name!

@dac09 dac09 merged commit 69c675d into main Jun 27, 2023
28 checks passed
@dac09 dac09 deleted the rc-js-deprecation-notice branch June 27, 2023 15:37
@redwoodjs-bot redwoodjs-bot bot removed this from the v6.0.0 milestone Jun 27, 2023
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 27, 2023
@jtoar jtoar modified the milestones: next-release, v6.0.0 Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants