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.

Sign up for GitHub

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

Jump to bottom

Improve file type detection #3497

Closed
wants to merge 2 commits into from
Closed

Improve file type detection #3497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcaae16
fix(middleware): remove query params and fragment identifiers from URLs
falsandtru May 5, 2020
8870cb5
refactor(middleware): handle paths as URLs
falsandtru May 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ const replaceWinPath = (path) => {

exports.normalizeWinPath = process.platform === 'win32' ? replaceWinPath : _.identity

exports.getFileType = (file) => {
return file.type || path.extname(file.path.split(/[?#]/, 1)[0] || '').substring(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any specific reason why we need || '' here?
Otherwise LGTM.

Copy link
Collaborator

@devoto13 devoto13 May 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also it may be better to first take extensions and then split as file name may contain ? or # (as it was before). E.g. myfile?.js or myfile#.js are valid filenames on Linux.

Hm, actually path.extname('http://example.com/mylib.js?load=test.md') will fail and result in md, not js. Probably we need a more robust logic here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any specific reason why we need || '' here?

String#split method can return an empty array.

Probably we need a more robust logic here...

Yes. I'm expecting that someone write a stricter implementation later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, looks like the source code treats paths only as URLs.

Copy link
Collaborator

@devoto13 devoto13 May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I'm expecting that someone write a stricter implementation later.

Well, current implementation breaks file type detection for myfile?.js and myfile#.js in favour of URLs. I don't think it is appropriate to break one corner case in favour of another.

Here is a better idea. Instead of having single method to handle both file path and URL we move the file type detection to constructors in file.js and url.js correspondingly. This way we can provide appropriate implementation depending on what we're dealing with.

E.g.

path.extname('src/myfile.js').substring(1) for file paths
path.extname(new URL('https://example.com/myfile.js?test=param').pathname).substring(1) for URLs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. You should do such refactoring yourself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okey, I'll try and submit an alternative PR in the coming days.

}

exports.mkdirIfNotExists = (directory, done) => {
// TODO(vojta): handle if it's a file
/* eslint-disable handle-callback-err */
Expand Down
41 changes: 20 additions & 21 deletions lib/middleware/karma.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
* - setting propert caching headers
*/

const path = require('path')
const url = require('url')
const helper = require('../helper')

Expand Down Expand Up @@ -163,31 +162,31 @@ function createKarmaMiddleware (

const scriptTags = []
for (const file of files.included) {
let filePath = file.path
const fileType = file.type || path.extname(filePath).substring(1)
const urlPath = file.isUrl
? file.path
: requestUrl === '/context.html'
? filePathToUrlPath(file.path, basePath, urlRoot, proxyPath) + '?' + file.sha
: filePathToUrlPath(file.path, basePath, urlRoot, proxyPath)
const fileType = helper.getFileType(file)

if (helper.isDefined(fileType) && !FILE_TYPES.includes(fileType)) {
log.warn(`Invalid file type (${fileType}), defaulting to js.`)
}

if (!file.isUrl) {
filePath = filePathToUrlPath(filePath, basePath, urlRoot, proxyPath)

if (requestUrl === '/context.html') {
filePath += '?' + file.sha
}
}

if (fileType === 'css') {
scriptTags.push(`<link type="text/css" href="${filePath}" rel="stylesheet">`)
} else if (fileType === 'dom') {
scriptTags.push(file.content)
} else if (fileType === 'html') {
scriptTags.push(`<link href="${filePath}" rel="import">`)
} else {
const scriptType = (SCRIPT_TYPE[fileType] || 'text/javascript')
const crossOriginAttribute = includeCrossOriginAttribute ? 'crossorigin="anonymous"' : ''
scriptTags.push(`<script type="${scriptType}" src="${filePath}" ${crossOriginAttribute}></script>`)
switch (fileType) {
case 'dom':
scriptTags.push(file.content)
break
case 'css':
scriptTags.push(`<link type="text/css" href="${urlPath}" rel="stylesheet">`)
break
case 'html':
scriptTags.push(`<link href="${urlPath}" rel="import">`)
break
default:
const scriptType = (SCRIPT_TYPE[fileType] || 'text/javascript')
const crossOriginAttribute = includeCrossOriginAttribute ? 'crossorigin="anonymous"' : ''
scriptTags.push(`<script type="${scriptType}" src="${urlPath}" ${crossOriginAttribute}></script>`)
}
}

Expand Down
9 changes: 9 additions & 0 deletions test/unit/helper.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,15 @@ describe('helper', () => {
})
})

describe('getFileType', () => {
it('should extract file type', () => {
expect(helper.getFileType({ path: 'a.js' })).to.equal('js')
expect(helper.getFileType({ path: 'a.js#' })).to.equal('js')
expect(helper.getFileType({ path: 'a.js?#' })).to.equal('js')
expect(helper.getFileType({ type: 'js', path: 'a' })).to.equal('js')
})
})

describe('mkdirIfNotExists', () => {
const fsMock = require('mocks').fs
const loadFile = require('mocks').loadFile
Expand Down