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/ open in VSCode for folder routes #142

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

muchisx
Copy link
Contributor

@muchisx muchisx commented Sep 5, 2024

Description

I've found that the open in VSCode button wasn't working due to the file extensions being missing on the exec call and also the feature not currently working with Remix's folder routes.

This PR is aimed for the main Open with VSCode feature in the "Active segments" section of the app.
image

I still believe there's a bit more fixing to do to the other implementations of this open-source method, but this is the first step.

This is a localized fix, so it doesn't affect the other implementations of open-source.

Before

Before-1.mp4

After

After-1.mp4

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I manually tested it, on Windows 11 23H3.
If there are more types of tests to be done I would appreciate some guidance as I'm a bit lost in that subject!

Checklist:

  • My code follows the guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@muchisx
Copy link
Contributor Author

muchisx commented Sep 14, 2024

@AlemTuzlak Hi! I don't have permissions to add you as reviewer, any thoughts on this?

@AlemTuzlak
Copy link
Contributor

@muchisx sorry, I've been on vacation the last 2 weeks, I'll try to sit down and go over this in detail on monday, from what I could see the code looks great

@omarsotillo
Copy link

Really looking forward to this 👏

return;
}

if (!source && routeID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the first part of the check is redundant as you already checked if the source exists above, so this can be if(routeID)

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the routeID isn't there? I would expect it to always be sent, or?

const filesInRemixPath = fs.readdirSync(remixDir);
const rootFile = filesInRemixPath.find((file) => reactExtensions.some((ext) => file === `root.${ext}`));

rootFile && exec(`code -g "${path.join(remixDir, rootFile)}${lineNum}"`);
Copy link
Contributor

Choose a reason for hiding this comment

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

extract this into a function called openInVsCode or something because here the path is not normalized and above it is,
something like:

function openInVsCode(path: string){
  exec(`code -g "${normalizePath(path)}"`˙)
}

if (isRoot) {
if (!fs.existsSync(remixDir)) return;
const filesInRemixPath = fs.readdirSync(remixDir);
const rootFile = filesInRemixPath.find((file) => reactExtensions.some((ext) => file === `root.${ext}`));
Copy link
Contributor

@AlemTuzlak AlemTuzlak Sep 17, 2024

Choose a reason for hiding this comment

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

can we also extract this and the lines 136-8 into a utility called findFileByExtension?

function findFileByExtension(prefix: string, filePaths: string[]){
  const file = files.find(file => allExtensions.some(ext => file === `${prefix}.${ext}`));
  return file
}

// Check if the path exists as a file with one of the given extensions
for (const ext of extensions) {
const filePath = `${routePath}${ext}`;
if (fs.existsSync(filePath) && fs.lstatSync(filePath).isFile()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the 2nd check is not needed because a folder wouldn't have an extension, I mean you could write it in but in practice I don't think anyone is doing something like folder.jsx/file.jsx or at least I'd hope not, or maybe even move this check into the return below, so type: fs.lstatSync(filePath).isFile() ? "file" : "directory"

Copy link
Contributor

@AlemTuzlak AlemTuzlak left a comment

Choose a reason for hiding this comment

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

I have some minor changes I'd like to see, if you're not free to do them I can merge it and do it myself. Let me know! Great work and thank you so much for this

@AlemTuzlak AlemTuzlak merged commit 14f2ca7 into forge-42:main Sep 17, 2024
@muchisx
Copy link
Contributor Author

muchisx commented Sep 17, 2024

@AlemTuzlak Awesome thanks! I left a few replies, I can give it some work later today but if you want to finish it you can go ahead!

@AlemTuzlak
Copy link
Contributor

@muchisx I finished it up myself as I'm closing shop for today and I won't have time to work on it for a while, thank you so much! Wanted to credit you on twitter but no idea what your name there is

@muchisx
Copy link
Contributor Author

muchisx commented Sep 17, 2024

🥳 Thank you!

It's same as gh, @muchisx !

kodiakhq bot referenced this pull request in technifit/tasker Sep 17, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [remix-development-tools](https://remix-development-tools.fly.dev/) ([source](https://github.com/Code-Forge-Net/Remix-Dev-Tools)) | [`4.4.1` -> `4.5.1`](https://renovatebot.com/diffs/npm/remix-development-tools/4.4.1/4.5.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/remix-development-tools/4.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/remix-development-tools/4.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/remix-development-tools/4.4.1/4.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/remix-development-tools/4.4.1/4.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>Code-Forge-Net/Remix-Dev-Tools (remix-development-tools)</summary>

### [`v4.5.1`](https://github.com/Code-Forge-Net/Remix-Dev-Tools/compare/v4.5.0...cd53418efa5fbae1b475605dc7e70222427dbaa7)

[Compare Source](https://github.com/Code-Forge-Net/Remix-Dev-Tools/compare/v4.5.0...cd53418efa5fbae1b475605dc7e70222427dbaa7)

### [`v4.5.0`](https://github.com/forge42dev/Remix-Dev-Tools/releases/tag/v4.5.0)

[Compare Source](https://github.com/Code-Forge-Net/Remix-Dev-Tools/compare/v4.4.1...v4.5.0)

#### What's Changed

-   Fix/ open in VSCode for folder routes by [@&#8203;muchisx](https://github.com/muchisx) in [https://github.com/forge42dev/Remix-Dev-Tools/pull/142](https://github.com/forge42dev/Remix-Dev-Tools/pull/142)

#### New Contributors

-   [@&#8203;muchisx](https://github.com/muchisx) made their first contribution in [https://github.com/forge42dev/Remix-Dev-Tools/pull/142](https://github.com/forge42dev/Remix-Dev-Tools/pull/142)

**Full Changelog**: forge-42/react-router-devtools@v4.4.1...v4.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/technifit/tasker).
@omarsotillo
Copy link

omarsotillo commented Sep 18, 2024

Hola! 👋 really cool to see this merged! just tried this morning updating to the new release version.

Sadly, for us, it does not work as expected and it even brings the server down when clicking on it. This is the error we get:

image

Let me know if you want me to open this as an issue

@AlemTuzlak
Copy link
Contributor

@omarsotillo can you install 4.5.2 and see if it crashes?

@omarsotillo
Copy link

Hello @AlemTuzlak it works now! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants