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

chore: remove abort-controller dependency #7044

Merged

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented Aug 3, 2023

AbortSignal is stable as of Node v15.4

https://nodejs.org/api/globals.html#class-abortcontroller


This PR is relying on #6108, so that one needs to be merged first

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

⚠️ No Changeset found

Latest commit: d112278

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 added the dependencies Pull requests that update a dependency file label Aug 3, 2023
@MichaelDeBoey MichaelDeBoey force-pushed the remove-abort-controller-dependency branch from f3c8d6e to 0b3cb8f Compare August 3, 2023 02:10
@brophdawg11
Copy link
Contributor

The CI error here look similar to other issues I've seen: #6296, #4371

 Type 'NodeOnDiskFile | undefined' is not assignable to type 'string | File | null | undefined'.

@MichaelDeBoey MichaelDeBoey force-pushed the remove-abort-controller-dependency branch from 0b3cb8f to 2c966b4 Compare August 3, 2023 15:27
@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Aug 3, 2023

@brophdawg11 Adding @types/node seems to have caused these 😕

We have these in #6108 as well, but I'm not sure how we should handle them though 🤔

@MichaelDeBoey
Copy link
Member Author

@brophdawg11 I managed to (temporarily) "fix" the type error with NodeOnDiskFile in #6108

Let's discuss there if this is an approach we like or not
Once merged, I'll rebase this one and this one should be fine again as well (I think)

@MichaelDeBoey MichaelDeBoey force-pushed the remove-abort-controller-dependency branch 3 times, most recently from b6f3998 to a1b97e6 Compare August 5, 2023 15:04
@MichaelDeBoey
Copy link
Member Author

I rebased this branch on #6108 to have passing tests, but that one should be merged first before merging this one

@brophdawg11 brophdawg11 removed their request for review August 7, 2023 15:42
@MichaelDeBoey MichaelDeBoey force-pushed the remove-abort-controller-dependency branch from 2a3da84 to d112278 Compare August 7, 2023 18:55
@jacob-ebey jacob-ebey merged commit 06fc77f into remix-run:dev Aug 7, 2023
9 checks passed
@MichaelDeBoey MichaelDeBoey deleted the remove-abort-controller-dependency branch August 7, 2023 19:26
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-06fc77f-20230808 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 Aug 9, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-a179aa7-20230809 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

🤖 Hello there,

We just published version v0.0.0-nightly-b1149bb-20230810 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
CLA Signed dependencies Pull requests that update a dependency file renderer:react runtime:node v2 Issues related to v2 apis
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants