remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.75k stars 2.51k forks source link

[BUG]: .DS_Store files in app/routes dir breaks app #2488

Closed jaidetree closed 2 years ago

jaidetree commented 2 years ago

What version of Remix are you using?

1.3.3

Steps to Reproduce

  1. Create a new remix repo
  2. Create a new folder in routes like routes/about
  3. Copy a .DS_Store file (on Mac OS X) or any non-standard encoded file into routes/about
  4. Run npx remix routes --json

Expected Behavior

Remix routes command should ignore the .DS_Store file

Actual Behavior

Remix throws an error:

Error: Invalid route module file: /home/runner/remix-ds-store-breaks-routes-cmd/app/routes/about/.DS_Store
    at /home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/config/routesConvention.js:74:11
    at visitFiles (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/config/routesConvention.js:214:7)
    at visitFiles (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/config/routesConvention.js:212:7)
    at Object.defineConventionalRoutes (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/config/routesConvention.js:63:3)
    at Object.readConfig (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/config.js:158:47)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Object.routes (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/cli/commands.js:136:18)
    at async Object.run (/home/runner/remix-ds-store-breaks-routes-cmd/node_modules/@remix-run/dev/cli/run.js:346:7)

Demo

Created a reproduction demo at https://replit.com/@eccentric-j/remix-ds-store-breaks-routes-cmd#package.json.

Solution

Was able to solve this by updating the ignoreRoutes in remix.config.js:

    ignoredRouteFiles: ['**/.*'],

Would you be open to a PR to make that default in https://github.com/remix-run/remix/blob/d9ae8e6478bcee326895b8e61a28a3e8d7f19fe7/templates/remix-ts/remix.config.js#L5?

machour commented 2 years ago

I don't think the suggested fix is valid, a route can begin with a ., like .well-known.

jaidetree commented 2 years ago

Then the default ".*" would not work with routes/.well-known and based on https://remix.run/docs/en/v1/api/conventions#escaping-special-characters, that route-file should be defined like [.]well-known.{mdx,js,tsx}

OliverM commented 2 years ago

This has bitten me for other files that appear in the routes directory too such as emac's undo-tree functionality that litters directories with a file suffixed with .~undo-tree~ for every edited file.

jdevries3133 commented 2 years ago

Certainly we can add .DS_Store, .-undo-tree-, and probably a few others to the default route ignore patterns, right?

kiliman commented 2 years ago

That's very subjective. Once you start adding to the default pattern, everyone's going to start wanting to include their special patterns, when it's simple to just do it yourself.

Perhaps an acceptable middle-ground is to use the .gitignore patterns as well. This will typically include patterns like .DS_Store or *.~undo-tree~ already. Since these files won't be committed, it makes sense that they are not valid route files either.

jdevries3133 commented 2 years ago

The only thing to note is that .DS_Store is often not in gitignore files. It'll be in the system git config for macs, or it'll be in .git/info/exclude.

Maybe we can have a boolean switch like gitIgnoreRoutes (I'm meh on that specific name) which only acknowledges route files that are in version control. That would take system wide config into account, as well as the .gitignore, plus things that might be in .git/info/exclude only.

The obvious sketchy thing about this is that the git repository often won't be present in certain deployment or packaging scenarios, I'd assume. At the same time, it should be impossible for files not in version control to make it into those environments, so maybe it's not a practical concern. For example, say that you gzip and scp your source without the git repo onto a remote machine. You accidentally grab a .DS_Store, too, and the deploy it in an environment without the repo, so the gitIgnorePaths feature doesn't work. The app then serves the .DS_Store in production. I'm unsure of whether this scenario is likely, but I'm sure there's plenty of precedent on problems like these that others will chime in.

Edit: ew gross, I didn't realize that GitHub removes all formatting from email replies, sorry.

On Mon, Apr 11, 2022, 10:23 AM Kiliman @.***> wrote:

That's very subjective. Once you start adding to the default pattern, everyone's going to start wanting to include their special patterns, when it's simple to just do it yourself.

Perhaps an acceptable middle-ground is to use the .gitignore patterns as well. This will typically include patterns like .DS_Store or *.~undo-tree~ already. Since these files won't be committed, it makes sense that they are not valid route files either.

— Reply to this email directly, view it on GitHub https://github.com/remix-run/remix/issues/2488#issuecomment-1095116326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN7GD5H2AZWV4DQ4STP3KD3VEQYUHANCNFSM5RSSIIBQ . You are receiving this because you commented.Message ID: @.***>

kiliman commented 2 years ago

I think the fact that you can edit remix.config to add your custom filters and with the new Stacks feature, where you can define your own "starter" templates, I'm not seeing a real need to clutter up the default config file. But that's just my take on it.

jdevries3133 commented 2 years ago

I don't have a good understanding of how many edge cases like the one I described would come from a git based route ignoring strategy.

I also don't have knowledge of additional edge cases of this sort of ignore strategy.

I feel that although ignore patterns certainly can do the job, I think that the multi-faceted approach of ignore files in git (system config, .git/info/exclude, and .gitignore... maybe others?) shows that ignoring files can be quite complex, and there's a demand for various solutions for different use cases. Like, for example, it's nice that the default git system config on Mac ignores .DS_Store files. That means that you don't need to take the time to remember to add .DS_Store to every repo you ever create -- that's good DX!

At the same time, a list of regexes is a feature with precedence in the js world. I've seen many other libraries use this convention, so maybe it's best to follow that trend.

I feel that a switch which hooks remix into git's ignore system would be a nice DX feature, if and only if the risk is low. If there is any risk of unwanted side effects, the minor DX improvement is easily washed away. Again, I feel that I have a poor understanding of the risk associated with this feature, so I'm grateful for any additional ideas on what some risks might be which I didn't mention previously.

chaance commented 2 years ago

Dotfiles are already set in the ignoredRouteFiles option for all templates, and we don't really want to start up-keeping a list of specific files here. If you need to enable a leading dot in your route for some reason you'll have to update your config accordingly and add whatever patterns you need to keep this from being a problem.

kiliman commented 2 years ago

Also leading dot in routes can be escaped [.]well-known

djscruggs commented 9 months ago

For anyone just finding this, the problem kept recurring for me, so I added the find & delete command to all my package.json scripts

"build": "find ./ -name \".DS_Store\" -print -delete; run-s build:*", "build:icons": "find ./ -name \".DS_Store\" -print -delete; tsx ./other/build-icons.ts", "build:remix": "find ./ -name \".DS_Store\" -print -delete; remix build --sourcemap", "build:server": "find ./ -name \".DS_Store\" -print -delete; tsx ./other/build-server.ts", "predev": "find ./ -name \".DS_Store\" -print -delete; npm run build:icons --silent", "dev": "find ./ -name \".DS_Store\" -print -delete; remix dev -c \"node ./server/dev-server.js\" --manual", `

quantuminformation commented 8 months ago

why is mac making these dumb files

quantuminformation commented 8 months ago

i still get that errors with https://github.com/remix-run/remix/blob/d9ae8e6478bcee326895b8e61a28a3e8d7f19fe7/templates/remix-ts/remix.config.js#L5 in my config:

 agcra-app git:(814-notify-officers) ✗ pdv

> agcra@ dev /Users/nikos/WebstormProjects/agcra-app
> remix dev --manual -c "arc sandbox -e testing"

 💿  remix dev

Error: Invalid route module file: app/routes/.DS_Store
    at /Users/nikos/WebstormProjects/agcra-app/node_modules/@remix-run/v1-route-convention/dist/index.js:69:11
    at visitFiles (/Users/nikos/WebstormProjects/agcra-app/node_modules/@remix-run/v1-route-convention/dist/index.js:235:7)
    at createRoutesFromFolders (/Users/nikos/WebstormProjects/agcra-app/node_modules/@remix-run/v1-route-convention/dist/index.js:59:3)
quantuminformation commented 8 months ago

wow my mac has loads

image
kiliman commented 7 months ago

why is mac making these dumb files

https://en.wikipedia.org/wiki/.DS_Store