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

Implement basic route-id data type & use remix config.appDirectory. #75

Merged
merged 6 commits into from
Jan 4, 2024

Conversation

dawnmist
Copy link
Contributor

@dawnmist dawnmist commented Jan 3, 2024

  • Addresses [Suggestion]: Typechecking route id's [Suggestion]: Typechecking route id's #39
  • Respects the remix config appDirectory setting
  • Calculates correct relative path to where config.appDirectory resides based on -o output directory.
  • Adds test with custom config.appDirectory path and multiple depth -o path.

Copy link

codesandbox bot commented Jan 3, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@yesmeck
Copy link
Owner

yesmeck commented Jan 3, 2024

@dawnmist thank you! Could you please fix the CI error?

@dawnmist
Copy link
Contributor Author

dawnmist commented Jan 4, 2024

It should be fixed now. :)

@dawnmist dawnmist changed the title Implement basic route-id data type generation. Implement basic route-id data type & use remix config.appDirectory. Jan 4, 2024
@dawnmist
Copy link
Contributor Author

dawnmist commented Jan 4, 2024

Is there a way to see what the actual Windows failures are (what the difference in the snapshot is)?

test("build customPaths routes", async () => {
await build(path.resolve(__dirname, "../../fixture/customPaths"), { outputDirPath: "./types/remix-routes", watch: false, strict: false });
expect(
fs.readFileSync(path.resolve( __dirname, "../../fixture/customPaths/types/remix-routes/remix-routes.d.ts"), "utf8"),
Copy link
Owner

Choose a reason for hiding this comment

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

You can try printing out the content of remix-routes.d.ts to see what's generated on windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next run should show that now.

I'm hoping it's not something like line-feed differences between linux & Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh. Sorry for the copy-paste error on the v1 route console log. Pushed the correct file path to log.

@@ -5,14 +5,30 @@ import { build } from '../cli';

test('build v1 routes', async () => {
await build(path.resolve(__dirname, '../../fixture/v1'), { outputDirPath: './node_modules', watch: false, strict: false });
console.log(fs.readFileSync(
path.resolve(__dirname, "../../fixture/v2/node_modules/remix-routes.d.ts"), "utf8"),
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
path.resolve(__dirname, "../../fixture/v2/node_modules/remix-routes.d.ts"), "utf8"),
path.resolve(__dirname, "../../fixture/v1/node_modules/remix-routes.d.ts"), "utf8"),

@dawnmist dawnmist force-pushed the add-route-id-validation branch 2 times, most recently from 1055829 to 8dc799f Compare January 4, 2024 02:49
@yesmeck
Copy link
Owner

yesmeck commented Jan 4, 2024

Great! CI is passing now. Could you please remove those temporary console.log statements, once that's done, we'll be ready to merge.

Addresses [Suggestion]: Typechecking route id's yesmeck#39
The function return is specifying it as a tuple, so 'as const' isn't needed to get typescript to treat it as a tuple, and is causing issues with readonly vs mutable types.
As it was being missed
@dawnmist
Copy link
Contributor Author

dawnmist commented Jan 4, 2024

Great! CI is passing now. Could you please remove those temporary console.log statements, once that's done, we'll be ready to merge.

I've done a rebase and removed the commit with the console logs. It should 🤞🏻 be good to go now.

@yesmeck yesmeck merged commit 2a7ec94 into yesmeck:master Jan 4, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 4, 2024
@yesmeck
Copy link
Owner

yesmeck commented Jan 4, 2024

Published as [email protected]. Cheers!

Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants