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

feat(create-remix): allow dots in nested folder names in GitHub repo shorthand #7277

Merged
merged 2 commits into from
Aug 29, 2023
Merged

feat(create-remix): allow dots in nested folder names in GitHub repo shorthand #7277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43e992a
Allow dots in nested folder names in github repo shorthand
brophdawg11 Aug 28, 2023
673889a
Add changeset
brophdawg11 Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/create-remix/__tests__/create-remix-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,25 @@ describe("create-remix CLI", () => {
expect(fse.existsSync(path.join(projectDir, "app/root.tsx"))).toBeTruthy();
});

it("works for GitHub username/repo/path combo (when dots exist in folder)", async () => {
let projectDir = getProjectDir("github-username-repo-path-dots");

let { status, stderr } = await execCreateRemix({
args: [
projectDir,
"--template",
"fake-remix-tester/nested-dir/folder.with.dots",
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 updated the nested-dir-repo.tar.gz file and copied the existing stack/ directory to folder.with-dots/ for this test

"--no-git-init",
"--no-install",
],
});

expect(stderr.trim()).toBeFalsy();
expect(status).toBe(0);
expect(fse.existsSync(path.join(projectDir, "package.json"))).toBeTruthy();
expect(fse.existsSync(path.join(projectDir, "app/root.tsx"))).toBeTruthy();
});

it("fails for GitHub username/repo/path combo when path doesn't exist", async () => {
let projectDir = getProjectDir("github-username-repo-path-missing");

Expand Down
Binary file modified packages/create-remix/__tests__/fixtures/nested-dir-repo.tar.gz
View file
Open in desktop
Binary file not shown.
6 changes: 5 additions & 1 deletion packages/create-remix/copy-template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,14 @@ function isValidGithubRepoUrl(
}

function isGithubRepoShorthand(value: string) {
if (isUrl(value)) {
return false;
}
Comment on lines +387 to +389
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 don't think we'd get collisions for URLs with file extensions - but it felt safe to ensure that we short circuit exit on non-RULs when looking for a shorthand

// This supports :owner/:repo and :owner/:repo/nested/path, e.g.
// remix-run/remix
// remix-run/remix/templates/express
return /^[\w-]+\/[\w-]+(\/[\w-]+)*$/.test(value);
// remix-run/examples/socket.io
return /^[\w-]+\/[\w-]+(\/[\w-.]+)*$/.test(value);
}

function isGithubReleaseAssetUrl(url: string) {
Expand Down
4 changes: 3 additions & 1 deletion packages/create-remix/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,9 @@ async function copyTempDirToAppDirStep(ctx: Context) {

let files1 = await getDirectoryFilesRecursive(ctx.tempDir);
let files2 = await getDirectoryFilesRecursive(ctx.cwd);
let collisions = files1.filter((f) => files2.includes(f));
let collisions = files1
.filter((f) => files2.includes(f))
.sort((a, b) => a.localeCompare(b));
Comment on lines -320 to +322
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 ran into a test failure due to these files being printed to the console in a different order, so sort the collisions list for stability


if (collisions.length > 0) {
let getFileList = (prefix: string) => {
Expand Down