- 
                Notifications
    
You must be signed in to change notification settings  - Fork 238
 
Web extension resources return incorrect MIME type #470
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
base: master
Are you sure you want to change the base?
Conversation
| 
           @amvanbaren I just found that mime types are listed in the  <?xml version="1.0" encoding="utf-8"?>
<Types xmlns="http://schemas.openxmlformats.org/package/2006/content-types">
	<Default Extension=".json" ContentType="application/json"/>
	<Default Extension=".vsixmanifest" ContentType="text/xml"/>
	<Default Extension=".png" ContentType="image/png"/>
	<Default Extension=".md" ContentType="text/markdown"/>
	<Default Extension=".js" ContentType="application/javascript"/>
	<Default Extension=".map" ContentType="application/json"/>
	<Default Extension=".txt" ContentType="text/plain"/>
	<Default Extension=".woff2" ContentType="font/woff2"/>
	<Default Extension=".html" ContentType="text/html"/>
	<Default Extension=".css" ContentType="text/css"/>
</Types> | 
    
| 
           Some more info https://docs.microsoft.com/en-us/visualstudio/extensibility/the-structure-of-the-content-types-dot-xml-file?view=vs-2022#attribute-name-attribute-1  | 
    
954393d    to
    cef3d08      
    Compare
  
    
          
 Then their docs are outdated 😅  | 
    
        
          
                server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAdapterTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 
           Need to test what happens when some files don't have file extension, e.g sometimes   | 
    
| 
           From my testing, @jeanp413, if there is no extension present, it will not be in the  This is just concerning web extensions, correct? So there will be no binaries served, which are the only thing in my mind that don't usually have extensions. cc @amvanbaren  | 
    
| 
           Thanks for testing this, @filiptronicek, from the MS docs anything else should be   | 
    
| var cssChunkWebResourceFile = new FileResource(); | ||
| cssChunkWebResourceFile.setExtension(extVersion); | ||
| cssChunkWebResourceFile.setName("extension/public/static/css/main.9cab4879.chunk.css"); | ||
| cssChunkWebResourceFile.setType(FileResource.RESOURCE); | ||
| cssChunkWebResourceFile.setStorageType(STORAGE_DB); | ||
| cssChunkWebResourceFile.setContent(".root { margin: 0 auto; }".getBytes()); | ||
| cssChunkWebResourceFile.setContentType("text/css"); | ||
| Mockito.when(repositories.findFileByTypeAndName(extVersion, FileResource.RESOURCE, "extension/public/static/css/main.9cab4879.chunk.css")) | ||
| .thenReturn(cssChunkWebResourceFile); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add one more test for the default mime type application/octet-stream
| 
           @amvanbaren I saw openvsx got finally deployed 🎉, can we merge this so it's shipped in next deployment 🙂  | 
    
          
 Yes, I'll rebase and complete this PR. 
 Up until PR #517 is deployed. Production is 25 commits behind   | 
    
0a5dbba    to
    d9e2b5e      
    Compare
  
    | 
           @amvanbaren let's merge 🙏  | 
    
          
 @jeanp413 Has this been reviewed and tested?  | 
    
| 
           I thought it was already approved 😅 , will take a look  | 
    
| 
           This PR is missing a migration: #468 (comment)  | 
    
| 
           @filiptronicek if you have some free time could you test this 🙏 ?  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
31aee8a    to
    9f36399      
    Compare
  
    9f36399    to
    1ca2479      
    Compare
  
    fe0f247    to
    180420d      
    Compare
  
    | 
           Same priority as #468  | 
    
180420d    to
    a673b85      
    Compare
  
    6880fa4    to
    d9d2a35      
    Compare
  
    Use [Content_Types].xml to set FileResource contentType
Add test case for default content type: application/octet-stream
Add migration to set FileResource.contentType
Set contentType for RESOURCE type
Remove dot ('.') if extension starts with a dot
Use utf-8 charset for text resources.
    d9d2a35    to
    9ef5023      
    Compare
  
    | 
           Ok, I've fixed the PR. @filiptronicek Do you have time to test it?  | 
    



Contributes to #468
Use Apache Tika to detect mimetype