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

fix(remix-dev): Update create-remix to use the new examples repository #4208

Merged
merged 6 commits into from
Sep 26, 2022

Conversation

machour
Copy link
Collaborator

@machour machour commented Sep 15, 2022

Also removed some now obsolete eslint rules

@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2022

🦋 Changeset detected

Latest commit: e241c5d

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix 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/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

@machour machour marked this pull request as draft September 15, 2022 22:46
@machour machour marked this pull request as ready for review September 15, 2022 23:24
@machour
Copy link
Collaborator Author

machour commented Sep 15, 2022

I'm hitting a snag here: My changes to create-remix works in reality, but as far as I understand the testing process, the mock is trying to read from the removed folder.

Any guidance would be appreciated :)

@chaance
Copy link
Collaborator

chaance commented Sep 16, 2022

Thanks @machour, I'll take a look at this tomorrow and see if I can help with the tests.

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.

Pressed approve too early—looks like we've got some failing test cases here. Let's get those patched up and I'll merge this for the next release.

Also: could we add a changeset noting the bug fix since this is broken at the moment?

@machour
Copy link
Collaborator Author

machour commented Sep 18, 2022

@chaance You were supposed to help for the tests 😅 (see your previous reply)

As commented, the problem is that tests were mocking the request to https://github.com/remix-run/remix/tree/main/examples by using the examples folder from the currently checked out repo in the workflow.

Now that we moved examples to their own repository, this mock isn't usable anymore.

I suppose I'll have to create a basic-example.tar.gz fixture and configure msw to correctly respond with that mock, but I've been trying and failing to make things work.

Can someone guide me, or take over this last step from me? (I'll understand what I was missing from the diff)

@MichaelDeBoey MichaelDeBoey force-pushed the chore/update-references-to-examples branch from 6c2932e to f2d25bc Compare September 19, 2022 09:36
@chaance
Copy link
Collaborator

chaance commented Sep 19, 2022

@machour looking at it this morning!

@mcansh
Copy link
Collaborator

mcansh commented Sep 26, 2022

taking a look into this now :)

Signed-off-by: Logan McAnsh <[email protected]>
@machour
Copy link
Collaborator Author

machour commented Sep 26, 2022

Thank you @mcansh !

@chaance
Copy link
Collaborator

chaance commented Sep 26, 2022

LGTM! @machour can we get conflicts resolved so I can merge this week? never mind on that, looks clean!

@chaance chaance merged commit 33e110d into remix-run:dev Sep 26, 2022
@machour machour deleted the chore/update-references-to-examples branch September 26, 2022 16:45
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-fb631c9-20220927 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 package:eslint-config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants