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.

Sign up for GitHub

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

Jump to bottom

feat: deprecate all functions & types exported from magicExports #3284

Merged
merged 1 commit into from
Nov 18, 2022
Merged

feat: deprecate all functions & types exported from magicExports #3284

merged 1 commit into from
Nov 18, 2022

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented May 22, 2022 •
edited
Loading

I wanted to re-submit #2735 again, as:

Original PR description:

As a follow-up of #2731, this will show deprecation warnings in the IDE + in console whenever people aren't using the ESLint config.

Inspired by @pcattori's remark (#2542 (comment)) on @mjackson's PR to rename createCloudflareKVSessionStorage to createWorkersKVSessionStorage (#2542).

Since each package is on it's own, I had to copy/paste the warn & getDeprecatedMessage functions over to each package.

If this somehow could be simplified (as this is doing the same thing over & over again), I'd be happy to do so.

@MichaelDeBoey MichaelDeBoey mentioned this pull request May 22, 2022
Merged
@MichaelDeBoey MichaelDeBoey requested a review from kentcdodds May 24, 2022 07:56
kentcdodds
kentcdodds approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I want @mjackson or @ryanflorence to approve this before merging.

@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 2 times, most recently from 0296fcf to 9546a52 Compare May 29, 2022 13:36
@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 4 times, most recently from 8c222f1 to 43c63ad Compare June 8, 2022 18:09
@wingleung wingleung mentioned this pull request Jun 9, 2022
Closed
2 tasks
@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 2 times, most recently from 8a3d496 to f30d803 Compare June 15, 2022 10:36
@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Jun 15, 2022 •
edited
Loading

@mjackson @ryanflorence Our ESLint config is warning with a deprecated message when importing from the remix package since 1.6.0, so I think we can merge this one as well now?
https://github.com/remix-run/remix/releases/tag/v1.6.0

@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch from f30d803 to 1b0e812 Compare June 17, 2022 11:01
@MichaelDeBoey MichaelDeBoey requested a review from kentcdodds June 17, 2022 12:48
@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch from 1b0e812 to d956a2c Compare June 21, 2022 15:54
@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 2 times, most recently from 1446ff5 to 3f5d0ed Compare September 18, 2022 14:15
@mjackson
Copy link
Member

I think now is a great time to officially deprecate using magic exports 👍

Do you know why the tests are failing in the diff view @MichaelDeBoey? Is that something to do with this PR? Or just something that has been happening for a while now? I'd like to clean that up if we can. If the test failures are unrelated to this PR, it's a huge distraction.

Can you also please say more about the approach you're taking here? It looks like you're generating a bunch of code in the build output. Is that right? I think I'd prefer to just write out the code (it's repetitive, I know) instead of introduce more complexity in our build.

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Sep 21, 2022 •
edited
Loading

Do you know why the tests are failing in the diff view?

@mjackson If you're talking about the failing tests underneath Unchanged files with check annotations, this has been happening for a while now.

Can you also please say more about the approach you're taking here? It looks like you're generating a bunch of code in the build output. Is that right? I think I'd prefer to just write out the code (it's repetitive, I know) instead of introduce more complexity in our build.

In #3543, @chaance removed all manual magicExports files & replaced it by generating them.
The generation script is taking the defined functions/types from rollup.config.js's getMagicExports & creates these magicExports files we had before.

My changes are building upon that script, by wrapping all exported functions in a warn function that's console.warning a deprecation message before calling the actual function (just like we do in #2542).

My changes also add an /** @deprecated **/ typehint to all functions/types & points them towards using the actual packages instead of using 'remix' package.

@chaance
Copy link
Collaborator

chaance commented Sep 21, 2022 •
edited
Loading

@mjackson We moved to code generation here because having the code in the workspace was causing TypeScript issues when we did our Rollup build overhaul a while back. It was always intended to be temporary as we planned on removing the exports altogether, but we went this route to move a little quicker, as that refactor was blocking other work (specifically, adopting Changesets as noted in #3543).

chaance
chaance requested changes Sep 21, 2022
View reviewed changes
Copy link
Collaborator

@chaance chaance left a comment

Choose a reason for hiding this comment

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

These look good overall. Can you merge the latest from dev one last time? Once that's taken care of I'll cut an experimental release to manually check the dev warnings, and if everything looks good we'll get this in for the next minor bump.

Thanks so much for seeing this through! 🙏

package.json Outdated Show resolved Hide resolved
rollup.utils.js Show resolved Hide resolved
.changeset/khaki-meals-prove.md Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 2 times, most recently from e3f28ca to 4caa205 Compare October 20, 2022 20:50
@chaance
Copy link
Collaborator

chaance commented Oct 20, 2022 •
edited
Loading

@MichaelDeBoey Just FYI, the only reason I haven't merged this yet is because I want to time it with an upcoming minor release. Once @brophdawg11 and @jacob-ebey finalize their work integrating React Router 6.4 I'll merge this and ship them together.

Just wanted you to know so you don't need to keep resolving conflicts until we're there!

@MichaelDeBoey MichaelDeBoey force-pushed the deprecate-magicExports branch 2 times, most recently from ca3f65d to 8c35380 Compare November 10, 2022 13:33
@chaance
Copy link
Collaborator

chaance commented Nov 18, 2022

@MichaelDeBoey Ready to merge this if you're able to bring the branch up-to-date 😄

@MichaelDeBoey
Copy link
Member Author

@chaance Rebase onto latest dev, so this should be OK to merge now 👍

@chaance chaance merged commit 848d8b0 into remix-run:dev Nov 18, 2022
@MichaelDeBoey MichaelDeBoey deleted the deprecate-magicExports branch November 18, 2022 18:45
@remix-run-bot remix-run-bot mentioned this pull request Nov 19, 2022
Closed
brophdawg11 added a commit that referenced this pull request Nov 21, 2022
@brophdawg11
Revert "Deprecate all functions & types exported from magicExports (#…
01f5ba5 
…3284)"

This reverts commit 848d8b0.
@brophdawg11 brophdawg11 mentioned this pull request Nov 21, 2022
Merged
brophdawg11 added a commit that referenced this pull request Nov 21, 2022
@brophdawg11
Revert "Deprecate all functions & types exported from magicExports (#…
e6a5373 
…3284)"

This reverts commit 848d8b0.
brophdawg11 added a commit that referenced this pull request Nov 21, 2022
@brophdawg11
Fix magic export deprecation messaging (#4661)
a5fb1f6 
* Revert "Deprecate all functions & types exported from `magicExports` (#3284)"

This reverts commit 848d8b0.

* Single deprecation warning per remix index file

* Revert "Single deprecation warning per remix index file"

This reverts commit 7774492.

* Add back in TS deprecation typings

* Warn on deprecated remix imports via esbuild plugin

* add chgangeset
@remix-run-bot remix-run-bot mentioned this pull request Nov 22, 2022
Closed
kentcdodds pushed a commit that referenced this pull request Dec 15, 2022
@MichaelDeBoey @kentcdodds
Deprecate all functions & types exported from magicExports (#3284)
00af729
kentcdodds pushed a commit that referenced this pull request Dec 15, 2022
@brophdawg11 @kentcdodds
Fix magic export deprecation messaging (#4661)
e00e6b7 
* Revert "Deprecate all functions & types exported from `magicExports` (#3284)"

This reverts commit 848d8b0.

* Single deprecation warning per remix index file

* Revert "Single deprecation warning per remix index file"

This reverts commit 7774492.

* Add back in TS deprecation typings

* Warn on deprecated remix imports via esbuild plugin

* add chgangeset
@mcansh mcansh mentioned this pull request Jan 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaance chaance chaance approved these changes

@mjackson mjackson Awaiting requested review from mjackson

@pcattori pcattori Awaiting requested review from pcattori

@jacob-ebey jacob-ebey Awaiting requested review from jacob-ebey

@ryanflorence ryanflorence Awaiting requested review from ryanflorence

@kentcdodds kentcdodds Awaiting requested review from kentcdodds

@brophdawg11 brophdawg11 Awaiting requested review from brophdawg11

Assignees

@mjackson mjackson

Labels
CLA Signed package:server-runtime renderer:react runtime:cloudflare runtime:node
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaelDeBoey @chaance @mjackson @kentcdodds