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(remix-dev): support a TypeScript file for the remix.init index script #2803

Merged
merged 9 commits into from
Jul 22, 2022
Merged

feat(remix-dev): support a TypeScript file for the remix.init index script #2803

merged 9 commits into from
Jul 22, 2022

Conversation

dvargas92495
Copy link
Contributor

Closes: #

  • Sorry, I just decided to make the PR instead of making a discussion first. I don't mind if the PR gets declined and find PRs a better context for relevant discussion.

  • Docs

    • Happy to add Docs if the maintainers agree with this change

  • Tests

The goal for this PR is to support Typescript for the Remix Init Stack script. I plan to have a pretty hefty Init script for my custom stack, and want to avoid writing that in JavaScript.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Apr 14, 2022

Hi @dvargas92495,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Apr 14, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

dvargas92495
dvargas92495 commented Apr 14, 2022
View reviewed changes
packages/remix-dev/__tests__/create-test.ts
"create",
projectDir,
"--template",
path.join(__dirname, "fixtures", "stack-init-ts.tar.gz"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the stack under /fixtures/stack, converting the index file to index.ts and console logging the message below in the default method exported

@MichaelDeBoey MichaelDeBoey changed the title Support a typescript file for the remix.init index script feat(remix-dev): support a TypeScript file for the remix.init index script Apr 14, 2022
@machour
Copy link
Collaborator

machour commented Apr 15, 2022

Hi @dvargas92495 and thank you for your submission!

Can you rebase your PR against the dev branch? Code changes need to target that branch.

@MichaelDeBoey MichaelDeBoey changed the base branch from main to dev April 15, 2022 11:22
@MichaelDeBoey MichaelDeBoey changed the base branch from dev to main April 15, 2022 11:22
@dvargas92495 dvargas92495 changed the base branch from main to dev April 16, 2022 02:12
@dvargas92495 dvargas92495 force-pushed the RemixInitTs branch 2 times, most recently from 3095dcc to 51a57ec Compare April 16, 2022 02:28
@dvargas92495
Copy link
Contributor Author

Just rebased it! Sorry for not reading directions 🙈

dvargas92495
dvargas92495 commented Apr 17, 2022
View reviewed changes
packages/remix-dev/cli/create.ts
@@ -188,7 +188,8 @@ async function extractLocalTarball(
throw Error(
"🚨 There was a problem extracting the file from the provided template.\n\n" +
` Template filepath: \`${filePath}\`\n` +
` Destination directory: \`${projectDir}\``
` Destination directory: \`${projectDir}\`\n` +
` ${err}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is passing locally and failing on this error. Happy to remove this after I find out why it's failing on GH actions, but I think the error itself should be somewhere in this error message

@dvargas92495
Copy link
Contributor Author

dvargas92495 commented Apr 19, 2022 •
edited
Loading

hey @machour, could we run tests on this PR? Says I need a maintainer to approve running workflows.

@dvargas92495
Copy link
Contributor Author

Okay this time I tar's the file with the script and ensure permissions were forgiving before tarring. Can we rerun the tests @machour?

@dvargas92495
Copy link
Contributor Author

any insight on why I'm still getting eaccess permission errors? I'm just copying the existing /stack fixture and then running the local tar.js script. Could it be related to me running windows locally?

@dvargas92495
Copy link
Contributor Author

@machour @MichaelDeBoey any thoughts on my question above?

@MichaelDeBoey
Copy link
Member

@dvargas92495 Could you please rebase on latest dev & resolve conflicts?

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 16, 2022
@dvargas92495
Copy link
Contributor Author

dvargas92495 commented May 16, 2022 •
edited
Loading

I was just able to get the tests passing on my repo: dvargas92495#1, so I no longer need help getting unblocked.

Going to rebase now and poke for review

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 16, 2022
@dvargas92495
Copy link
Contributor Author

Ok rebased! @MichaelDeBoey could you run the tests?

Turns out the failure was resolved by doing the same steps to tar the directory on a mac instead of a windows... 🤷

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented May 16, 2022 •
edited
Loading

@dvargas92495 Tests seem to fail on CI.
Can you have a look at them please?

@dvargas92495
Copy link
Contributor Author

Whoops sorry! Left a typo on the rebase, could we rerun?

@dvargas92495
Copy link
Contributor Author

Nice, @machour + @MichaelDeBoey PR is finally ready for review!

@MichaelDeBoey MichaelDeBoey requested a review from mcansh May 16, 2022 19:38
@dvargas92495
Copy link
Contributor Author

Hey @mcansh, anything I could do on my end to get this ready for review?

@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 054ccfb

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

@dvargas92495
Copy link
Contributor Author

Hey @machour and @MichaelDeBoey I just rebased again could we get tests to run?

Once they run, is there anything I could do to help get it reviewed? Should I open a discussion? Trying to get a sense of what the blockers are

kentcdodds
kentcdodds approved these changes Jul 22, 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.

Thank you for your hard work on this @dvargas92495. I'm a fan 👍 Thanks a bunch!

@kentcdodds kentcdodds merged commit 0ee00ff into remix-run:dev Jul 22, 2022
@dvargas92495 dvargas92495 deleted the RemixInitTs branch July 22, 2022 21:46
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-3965c06-20220723 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
Reviewers

@kentcdodds kentcdodds kentcdodds approved these changes

@mcansh mcansh Awaiting requested review from mcansh

Assignees
No one assigned
Labels
CLA Signed feat:typescript package:dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dvargas92495 @machour @MichaelDeBoey @kentcdodds