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

feat(server): dev safety checks #1489

Merged
merged 7 commits into from
Jul 31, 2023

Conversation

mbhrznr
Copy link
Contributor

@mbhrznr mbhrznr commented Jul 19, 2023

closes #1416
closes #1524

as discussed in the aforementioned issue, @deer was so kind to let me finish this one off.
as expected it gave me quite some nice understanding of fresh's internals.

this pr introduces the following check when being in dev mode:

  • only one _app.tsx is allowed per application and it must have default export
  • one 500.tsx and it must have default export
  • one 404.tsx and it must have default export
  • pascal case or kebab case islands (island name) (this one became obsolete w/ 1.3)
  • route must have handler or component
  • route patterns must be unique
  • if static override, ensure there is no normal static directory
  • static file conflict with routes (because static files have higher priority)
  • plugin doesn't call render
  • plugin each defined module must be a js/ts module

the functionality can be tested by navigating into tests/fixture_dev_checks/ and running deno task start

@mbhrznr mbhrznr force-pushed the 1416_safety_checks branch from add58ea to cd42d46 Compare July 19, 2023 20:24
@mbhrznr mbhrznr force-pushed the 1416_safety_checks branch from 9291352 to d71919c Compare July 28, 2023 18:36
@deer
Copy link
Contributor

deer commented Jul 31, 2023

Thanks for finishing this up. Looks pretty good, but I wonder if we want to specifically create a union type for all the categories and then reference them as something like this:

return [{
      category: DevChecks.ModuleExport,
      message: `Your ${moduleName} file does not have a default export.`,
      fileLink: moduleName,
    }];

instead of

return [{
      category: "Module Export",
      message: `Your ${moduleName} file does not have a default export.`,
      fileLink: moduleName,
    }];

Additionally, what about implementing the check in #1414? And taking the tests as well. Then I can just close that one, and a maintainer can close #539. And then you can resolve three issues at once (the two you mention, plus 475 which I was going to resolve).

@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jul 31, 2023

@deer thanks for the suggestion. will incorporate everything you mentioned!

Copy link
Collaborator

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding these 🎉

@marvinhagemeister marvinhagemeister merged commit a7fe9d0 into denoland:main Jul 31, 2023
marvinhagemeister added a commit that referenced this pull request Aug 1, 2023
feat(server): add safety checks during `dev` (#1489)
@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Aug 1, 2023

Apologies for the churn. I merged the PR too quickly and it's currently blocking me on feature work, so I've reverted it in #1567. So far I encountered that the handlers rule doesn't account for them being an array.

The output is a bit messy when there are multiple errors. Maybe some visual hierarchy via colors can help?

Most rules don't print the file name where the error happened which makes it difficult to track down where the error is coming from. The route.name property that many rules rely on is not the file name.

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