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

fix(client): Enable loading different file types when running in parent mode without iframe #3289

Merged
merged 8 commits into from Apr 12, 2019
Merged

fix(client): Enable loading different file types when running in parent mode without iframe #3289

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d39167c
fix(client): Includes attributes like type in script tags when runnin…
chan1cyrus2 Mar 20, 2019
cd0bf3c
fix(client): Enable loading different file types when running in pare…
chan1cyrus2 Mar 20, 2019
5fd685a
fix(client): Update comments on escaping character with special roles…
chan1cyrus2 Mar 22, 2019
699f8a5
fix(client): Fix comments
chan1cyrus2 Mar 22, 2019
cddb93c
fix(client): Use conditions to make the require changes.
Apr 12, 2019
43d9629
fix(client): Fix CLA
chan1cyrus2 Apr 12, 2019
905e464
fix(client): Fix CLA second try!
chan1cyrus2 Apr 12, 2019
e87e349
fix(client): Fix typo
chan1cyrus2 Apr 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 18 additions & 2 deletions client/karma.js
View file
Open in desktop
Expand Up @@ -66,8 +66,24 @@ function Karma (socket, iframe, opener, navigator, location) {
} else if (url !== 'about:blank') {
var loadScript = function (idx) {
if (idx < window.__karma__.scriptUrls.length) {
var ele = document.createElement('script')
ele.src = window.__karma__.scriptUrls[idx]
var parser = new DOMParser()
// Browsers don't like character <> in string when assigning
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than "inanimate object does not like" can we say eg
// Escape characters with special roles in HTML that appear in the body of tags.

But I don't understand this. How can a DOM parser not support angle bracket parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say we have only one file (bar/foo.js). We then construct this element that would like to add to the dom (<script type="text/javascript" src="/bar/foo.js" ></script>). When we embed this string into %SCRIPT_URL_ARRAY% in client_with_context.html. The html file will have a line look like this: window.karma.scriptUrls = ["<script type="text/javascript" src="/bar/foo.js" ></script>"]. When the browser parse the html file, they stop parsing once it reaches the character "<". They parse it and expect it as an open tag even though it is wrapped with double quotation. This happens on both Chrome and the lightweight browser I am using. Therefore I turn them to hex code which looks like this in the html: window.karma.scriptUrls = ["\x3Cscript type="text/javascript" src="/bar/foo.js" \x3E\x3C/script\x3E"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be clearer to have the template be an array of objects:
[{type: "text/javascript", src: "/bar/foo.js" }, {...}] Then in this code we walk the object and create the script tags. So no angle brackets in the template means no reason to encode then replace the angle brackets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about this approach too. However, the script tags html strings are constructed case by case in the middleware.

if (fileType === 'css') {
              scriptTags.push(`<link type="text/css" href="${filePath}" rel="stylesheet">`)
            } else if (fileType === 'dom') {
              scriptTags.push(includedContent.get(file.path))
            } 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>`)
            }
          }

If this changes in the future, we have to update the logic to construct and reconstruct the script tag object too. I think that actually adds another layer of complexity and maintenance. Therefore, I prefer just escaping and de-escaping the generated string here (which I think also have closest logic to the regular case).

// stringify json into a variable, so we replace them with escape
// hex
var string = window.__karma__.scriptUrls[idx]
.replace(/\\x3C/g, '<')
.replace(/\\x3E/g, '>')
var doc = parser.parseFromString(string, 'text/html')
var ele = doc.head.firstChild || doc.body.firstChild
// script elements created by DomParser is marked as unexecutable,
chan1cyrus2 marked this conversation as resolved.
Show resolved Hide resolved
// create a new script element manually and copy necessary properties
// so it is executable
if (ele.tagName && ele.tagName.toLowerCase() === 'script') {
var tmp = ele
ele = document.createElement('script')
ele.src = tmp.src
ele.crossOrigin = tmp.crossorigin
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the different case significant? If yes please add a comment; if no please pick one.

}
ele.onload = function () {
loadScript(idx + 1)
}
Expand Down
5 changes: 3 additions & 2 deletions lib/middleware/karma.js
View file
Open in desktop
Expand Up @@ -192,8 +192,6 @@ function createKarmaMiddleware (
}
}

scriptUrls.push(filePath)

if (fileType === 'css') {
scriptTags.push(`<link type="text/css" href="${filePath}" rel="stylesheet">`)
} else if (fileType === 'dom') {
Expand All @@ -206,6 +204,9 @@ function createKarmaMiddleware (
scriptTags.push(`<script type="${scriptType}" src="${filePath}" ${crossOriginAttribute}></script>`)
}
}
for (const script of scriptTags) {
scriptUrls.push(script.replace(/</g, '\\x3C').replace(/>/g, '\\x3E'))
chan1cyrus2 marked this conversation as resolved.
Show resolved Hide resolved
}

const mappings = data.includes('%MAPPINGS%') ? files.served.map((file) => {
const filePath = filePathToUrlPath(file.path, basePath, urlRoot, proxyPath)
Expand Down