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

Use content-dispositon package #820

Closed
Prinzhorn opened this issue Aug 23, 2021 · 5 comments · Fixed by #844
Closed

Use content-dispositon package #820

Prinzhorn opened this issue Aug 23, 2021 · 5 comments · Fixed by #844

Comments

@Prinzhorn
Copy link
Contributor

https://www.npmjs.com/package/content-disposition

These three occurrences would benefit from using a proper package. Because the format is not as trivial as they make it look.

http.response.setHeader('Content-Disposition', dispositionType + dispositionName + dispositionEncoding);

https://github.com/veliovgroup/Meteor-Files/blob/master/docs/gridfs-bucket-integration.md#4-create-download-handler
https://github.com/veliovgroup/Meteor-Files/blob/7f67a0885e120ad77085b191e3399add575a2d89/docs/gridfs-streaming.md

E.g. this will break easily:

http.response.setHeader('Content-Disposition', `inline; filename="${file.name}"`);
@dr-dimitru
Copy link
Member

@Prinzhorn is it broken now? Or you just suggesting a package?

@Prinzhorn
Copy link
Contributor Author

I didn't set up an actual PoC, but every edge case the package handles should be broken. E.g. if file.name has a double quote. This example doesn't do any special handling https://github.com/veliovgroup/Meteor-Files/blob/master/docs/gridfs-bucket-integration.md#4-create-download-handler which I think sparked this issue. The other places seem to handle edge cases, I was just advocating re-using the same code across the repo.

@dr-dimitru
Copy link
Member

@Prinzhorn thank you for the prompt reply. I believe we should update documentation in that case. Package itself does everything necessary to meet RFC 6266

@Prinzhorn
Copy link
Contributor Author

Sounds good, I think the documentation is the only part that needs to be adjusted then!

@dr-dimitru
Copy link
Member

@Prinzhorn I see that you're the last one to edit this file, would you be open to send a PR?

dr-dimitru added a commit that referenced this issue Jun 29, 2022
__Breaking Changes__

- ⚠️ Changes in `namingFunction`, — now naming function acts the same on the Client and Server, upon insert, load, and write. Test your implementation with changed logic. Output of Server function supersedes Client's function output

__Changes__

- 📔 Merge #843 and fix #820, thanks to @Prinzhorn
- 📔 Documentation refactoring focused on examples and its simplifications
- 👨‍💻 Support nested custom path returned from `namingFunction`
- 👨‍💻 Fix `namingFunction` behavior on Client and Server in upload, load, and write methods, closing #842; Thanks to @nchrschae
- 👷‍♂️ Now library exports its helpers `import { FilesCollection, helpers };`
- 👷‍♂️ Add `.meteorignore` to minimize package's footprint
@dr-dimitru dr-dimitru mentioned this issue Jun 29, 2022
dr-dimitru added a commit that referenced this issue Jun 29, 2022
📦 v2.2.0

__Breaking Changes__

- ⚠️ Changes in `namingFunction`, — now naming function acts the same on the Client and Server, upon insert, load, and write. Test your implementation with changed logic. Output of Server function supersedes Client's function output

__Changes__

- 📔 Merge #843 and fix #820, thanks to @Prinzhorn
- 📔 Documentation refactoring focused on examples and its simplifications
- 👨‍💻 Support nested custom path returned from `namingFunction`
- 👨‍💻 Fix `namingFunction` behavior on Client and Server in upload, load, and write methods, closing #842; Thanks to @chrschae
- 👷‍♂️ Now library exports its helpers `import { FilesCollection, helpers };`
- 👷‍♂️ Add `.meteorignore` to minimize package's footprint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants