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

feat(remix-dev): add warning when future.v2_routeConvention is not enabled #5606

Merged
merged 6 commits into from
Mar 6, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Feb 28, 2023

for some reason adding the warning is causing the HMR tests to fail 🤔 https://github.com/remix-run/remix/actions/runs/4317024896/jobs/7533572210

edit: it was checking stderr length and the warning goes to stderr 🤦‍♂️

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2023

🦋 Changeset detected

Latest commit: 47a9ace

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
remix Patch
@remix-run/dev Patch
create-remix Patch
@remix-run/css-bundle Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcansh mcansh force-pushed the logan/deprecate-old-route-convention branch 3 times, most recently from 82b723e to b75bc13 Compare March 1, 2023 22:28
mcansh added 2 commits March 2, 2023 13:18
… enabled

Signed-off-by: Logan McAnsh <[email protected]>

chore: update warning

test: update test to include v2_routeConvention error  message

Signed-off-by: Logan McAnsh <[email protected]>

update warning

Signed-off-by: Logan McAnsh <[email protected]>
@mcansh mcansh force-pushed the logan/deprecate-old-route-convention branch from b75bc13 to d9a0228 Compare March 2, 2023 18:20
… the warning ending up in stderr

Signed-off-by: Logan McAnsh <[email protected]>
@mcansh mcansh force-pushed the logan/deprecate-old-route-convention branch from b972de8 to d9b9049 Compare March 6, 2023 16:20
@mcansh mcansh changed the title chore(remix-dev): add warning when future.v2_routeConvention is not enabled feat(remix-dev): add warning when future.v2_routeConvention is not enabled Mar 6, 2023
@@ -727,3 +732,4 @@ let listFormat = new Intl.ListFormat("en", {
});

export let serverBuildTargetWarning = `The "serverBuildTarget" config option is deprecated. Use a combination of "publicPath", "serverBuildPath", "serverConditions", "serverDependenciesToBundle", "serverMainFields", "serverMinify", "serverModuleFormat" and/or "serverPlatform" instead.`;
export let flatRoutesWarning = `The old route convention has been deprecated in favor of "flat routes". Please enable it via your remix.config https://remix.run/docs/en/main/file-conventions/route-files-v2`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we prefix this with `⚠️ DEPRECATED: or something to make it stand out a bit more?

Suggested change
export let flatRoutesWarning = `The old route convention has been deprecated in favor of "flat routes". Please enable it via your remix.config https://remix.run/docs/en/main/file-conventions/route-files-v2`;
export let flatRoutesWarning = `⚠️ DEPRECATED: The old nested folders route convention has been deprecated in favor of "flat routes. Please enable the new routing convention via the \`future.v2_routeConvention\` flag in your \`remix.config.js\` file. For more information, please see https://remix.run/docs/en/main/file-conventions/route-files-v2.`;

Eventually I'd love to see it in yellow via chalk or something but I know we have future CLI improvements in the pipeline so probably worth waiting for that before doing anything too wild.

Copy link
Contributor

@brophdawg11 brophdawg11 Mar 6, 2023

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 also make sure we have a section in the new docs on "here's how to continue using the old nested folders version via defineRoutes"

Copy link
Collaborator Author

@mcansh mcansh Mar 6, 2023

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 also make sure we have a section in the new docs on "here's how to continue using the old nested folders version via defineRoutes"

agreed, i ripped out the old convention as @mcansh/remix-v1-routes, but there may be further tweaking needed once #5649 and #5634 are done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eventually I'd love to see it in yellow via chalk or something but I know we have future CLI improvements in the pipeline so probably worth waiting for that before doing anything too wild.

we can do this if we either make all warnOnce uses yellow or an arg passed to it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all uses of warnOnce could benefit from being yellow imo

  • sessions not being signed
  • cookies have expires setup
  • flat routes v2
  • serverBuildTarget

@mcansh mcansh merged commit 712732f into dev Mar 6, 2023
@mcansh mcansh deleted the logan/deprecate-old-route-convention branch March 6, 2023 18:36
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Mar 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-e78a09b-20230307 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants