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

templates: have loadContext available in handleRequest #6190

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented Apr 26, 2023

Follow-up of #5836

@changeset-bot
Copy link

changeset-bot bot commented Apr 26, 2023

⚠️ No Changeset found

Latest commit: e5aa0c2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@MichaelDeBoey MichaelDeBoey force-pushed the have-loadContext-available-in-handleRequest-in-templates branch 2 times, most recently from 4bd8a14 to 1c97d7f Compare April 28, 2023 21:37
@kiliman
Copy link
Collaborator

kiliman commented Apr 28, 2023

Any chance we can fix the "typo" before v2 launches?

getLoadContext should be getLoaderContext or, even better, getAppContext.

@MichaelDeBoey MichaelDeBoey marked this pull request as ready for review May 1, 2023 20:13
@MichaelDeBoey MichaelDeBoey force-pushed the have-loadContext-available-in-handleRequest-in-templates branch 3 times, most recently from 4ab3f8a to 052f2f6 Compare May 6, 2023 15:37
@brophdawg11
Copy link
Contributor

@kiliman I think getLoadContext is going to change once we add the middleware concepts in v3, so that rename/adjustment should already be on the roadmap for v3

@MichaelDeBoey MichaelDeBoey force-pushed the have-loadContext-available-in-handleRequest-in-templates branch 2 times, most recently from c0cda75 to 9b22c73 Compare May 15, 2023 20:37
@brophdawg11
Copy link
Contributor

@MichaelDeBoey I don't care enough to push hard for it but I still don't like the _ prefix in our templates. I think if we are going to include the parameter in the templates so the user knows it's available (I'm not 100% sold we even need to since they can find out its availability via TS if they start using getLoadContext), then it should be up to them how to name it if they follow some set of guidelines for naming unused vars. Most of the time I think they'll just delete it but at least they know it's there.

If we keep the _ then they need to rename the var down the road if they decide to leverage it which feels odd.

I also don't like that the _ seems to disable the faded color VSCode adds to unused vars and almost makes it look like it is used?

Screenshot 2023-05-22 at 10 49 44 AM Screenshot 2023-05-22 at 10 47 51 AM

@MichaelDeBoey MichaelDeBoey force-pushed the have-loadContext-available-in-handleRequest-in-templates branch from 9b22c73 to e5aa0c2 Compare May 25, 2023 22:42
@MichaelDeBoey MichaelDeBoey merged commit f0e4b94 into remix-run:main May 25, 2023
@MichaelDeBoey MichaelDeBoey deleted the have-loadContext-available-in-handleRequest-in-templates branch May 25, 2023 22:44
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

🤖 Hello there,

We just published version 1.17.0-pre.0 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!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-45a91e0-20230603 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!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

🤖 Hello there,

We just published version 1.17.0-pre.1 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!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

🤖 Hello there,

We just published version 1.17.0 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants