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

fix: Provide correct mime type for javascript and css #504

Merged
merged 4 commits into from
May 19, 2023

Conversation

Artur-
Copy link
Contributor

@Artur- Artur- commented Feb 6, 2023

The correct mimetype for javascript is a requirement when loading javascript modules

@deki deki self-assigned this Feb 6, 2023
@deki
Copy link
Collaborator

deki commented Feb 6, 2023

@Artur- Agree that this needs to be fixed. I've changed the implementation to be more generic instead of hard-coding two types. Could you please have a look?

@Artur-
Copy link
Contributor Author

Artur- commented Feb 6, 2023

It does not seem to work for me. Files.probeContentType returns null for a filename like /VAADIN/build/app.afe73ca6.js

@deki
Copy link
Collaborator

deki commented Feb 6, 2023

Hmm can you replicate that in your testcase? Because that succeeds with the implementation...

@Artur-
Copy link
Contributor Author

Artur- commented Feb 6, 2023

It works on my local JVM, fails when deployed

@deki
Copy link
Collaborator

deki commented Feb 8, 2023

Hmm ok did some more testing:

  • Corretto 8.362.08.1 - fail
  • Corretto 11.0.18.10.1 - success
  • Corretto 17.0.6.10.1 - success

Which runtime are you using?

@Artur-
Copy link
Contributor Author

Artur- commented Feb 8, 2023

image

@deki
Copy link
Collaborator

deki commented Feb 8, 2023

Ok I was able to reproduce it. Looks like the method is highly platform specific but I want to avoid pulling in another library like Apache Tika to solve it.
Will get in touch with our Lambda team internally...

@deki
Copy link
Collaborator

deki commented Feb 8, 2023

Made another change, this implementation should be close to what you proposed (using the filename to guess the context type).

@Artur-
Copy link
Contributor Author

Artur- commented Feb 9, 2023

Still does not work in my case:

With file being e.g. /VAADIN/build/react.8caaa40e.js I see
mimeTypes.getContentType(file): application/octet-stream
mimeTypeGuess: null

and the result is then application/octet-stream

@deki
Copy link
Collaborator

deki commented Feb 13, 2023

It turned out the related mailcap package is present on Amazon Linux but not part of the Lambda managed runtime. Still waiting for feedback from our service team, afterwards we'll make a decision on how to fix it.

@Artur-
Copy link
Contributor Author

Artur- commented Apr 3, 2023

Any luck in couple of months? :)

@deki
Copy link
Collaborator

deki commented Apr 3, 2023

Our Lambda service team confirmed they will fix it but I can't tell a date yet.

Afterwards /etc/mime.types will be present and https://github.com/corretto/corretto-11/blob/b99712f303f94fffcb8af283e80a3c71f9a1ae0b/src/java.base/linux/classes/sun/nio/fs/LinuxFileSystemProvider.java#L107 will work as designed.

Artur- and others added 4 commits May 19, 2023 11:02
The correct mimetype for javascript is a requirement when loading javascript modules#
…les.probeContentType returns null on Lambda)
…ntent type cannot be resolved (as per servlet-api docs), a fix to make probeContentType work in Lambda execution environment is being worked on by the Lambda service team
@deki
Copy link
Collaborator

deki commented May 19, 2023

Starting with 2.0 we return null instead of application/octet-stream if content type cannot be resolved (as per servlet-api docs), a fix to make probeContentType work in Lambda execution environment is being worked on by the Lambda service team.
I opened #562 to improve test coverage.

@deki deki merged commit f5cf12d into aws:main May 19, 2023
4 checks passed
@deki
Copy link
Collaborator

deki commented Nov 20, 2023

The Java 21 managed runtime that was just released contains the necessary fix to make probeContentType work as well.

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.

None yet

2 participants