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

error when a dataloader has a single extension #206

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Nov 18, 2023

this seems like good practice:

  • matches the documentation
  • warns the user, and prevents outputting payloads with no extension
  • slightly increases security (if your dev server is public, it won't be used to run ANY .sh file in your docs directory, these files now need a double extension)

@Fil Fil requested a review from mbostock November 18, 2023 18:06
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Good catch!

To confirm my understanding, the bug is: the user requested the extension-less file foo; this file did not exist, so we then (inadventantly) checked for a single-extension data loader such as foo.js (etc.), and found it. But we intended that data loaders should have a double extension, so we should not have considered foo.js a data loader.

I think the fix here is not to error but to return undefined above the for loop. The proposed error implies that the user intended for foo.ts to be a data loader, but that’s not necessarily true and I think a 404 is more expected and neutral: the user asked for foo and it didn’t exist, and it was just a bug in our implementation that we considered foo.ts a data loader.

@Fil
Copy link
Contributor Author

Fil commented Nov 18, 2023

I came to it the other way (by writing a data loader without a double extension), but I agree that the scenario you're mentioning is more common. We can add a log, though, to help in both cases.

@Fil Fil requested a review from mbostock November 20, 2023 16:21
@Fil Fil enabled auto-merge (squash) November 20, 2023 18:24
@Fil Fil merged commit 36d794d into main Nov 20, 2023
1 check passed
@Fil Fil deleted the fil/loader-double-extension branch November 20, 2023 18:25
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.

2 participants