Skip to content

Commit

Permalink
fix(rsg): Support relative paths in <script/> tags (#474)
Browse files Browse the repository at this point in the history
RBI allows relative paths in <script/> tags, resolved relative to the
XML file that contains the element.  We should support that too, because
SGDEX makes heavy use of relative paths (also it's really handy :D)
  • Loading branch information
sjbarag authored Jul 27, 2020
1 parent 54fdf8c commit e2cf3a9
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 21 deletions.
17 changes: 14 additions & 3 deletions src/Error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,22 @@ export class BrsError extends Error {
* and column, and the message associated with the error, e.g.:
*
* `lorem.brs(1,1-3): Expected '(' after sub name`
* ```
* @see BrsError#format
*/
format() {
let location = this.location;
return BrsError.format(this.message, this.location);
}

/**
* Formats a location and message into a human-readable string including filename, starting
* and ending line and column, and the message associated with the error, e.g.:
*
* `lorem.brs(1,1-3): Expected '(' after sub name`
*
* @param message a string describing the error
* @param location where the error occurred
*/
static format(message: string, location: Location): string {
let formattedLocation: string;

if (location.start.line === location.end.line) {
Expand All @@ -28,7 +39,7 @@ export class BrsError extends Error {
formattedLocation = `${location.file}(${location.start.line},${location.start.column},${location.end.line},${location.end.line})`;
}

return `${formattedLocation}: ${this.message}\n`;
return `${formattedLocation}: ${message}\n`;
}
}

Expand Down
68 changes: 57 additions & 11 deletions src/componentprocessor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import pSettle = require("p-settle");
const readFile = promisify(fs.readFile);
import * as fg from "fast-glob";
import { Environment } from "../interpreter/Environment";
import { BrsComponentName } from "../brsTypes";
import { BrsError } from "../Error";

interface FieldAttributes {
id: string;
Expand Down Expand Up @@ -89,11 +89,12 @@ export async function getComponentDefinitionMap(rootDir: string) {
let defs = xmlFiles.map((file) => new ComponentDefinition(file));
let parsedPromises = defs.map(async (def) => def.parse());

return processXmlTree(pSettle(parsedPromises));
return processXmlTree(pSettle(parsedPromises), rootDir);
}

async function processXmlTree(
settledPromises: Promise<pSettle.SettledResult<ComponentDefinition>[]>
settledPromises: Promise<pSettle.SettledResult<ComponentDefinition>[]>,
rootDir: string
) {
let nodeDefs = await settledPromises;
let nodeDefMap = new Map<string, ComponentDefinition>();
Expand Down Expand Up @@ -151,13 +152,13 @@ async function processXmlTree(
}
});

nodeDefMap.forEach((nodeDef) => {
for (let nodeDef of nodeDefMap.values()) {
let xmlNode = nodeDef.xmlNode;
if (xmlNode) {
nodeDef.children = getChildren(xmlNode);
nodeDef.scripts = getScripts(xmlNode);
nodeDef.scripts = await getScripts(xmlNode, nodeDef.xmlPath, rootDir);
}
});
}

return nodeDefMap;
}
Expand Down Expand Up @@ -241,19 +242,64 @@ function parseChildren(element: XmlElement, children: ComponentNode[]): void {
});
}

function getScripts(node: XmlDocument): ComponentScript[] {
async function getScripts(
node: XmlDocument,
xmlPath: string,
rootDir: string
): Promise<ComponentScript[]> {
let scripts = node.childrenNamed("script");
let componentScripts: ComponentScript[] = [];

// TODO: Verify if uri is valid
scripts.map((script) => {
for (let script of scripts) {
let absoluteUri: URL;
try {
absoluteUri = new URL(script.attr.uri, `pkg:/${path.posix.relative(rootDir, xmlPath)}`);
} catch (err) {
let file = await readFile(xmlPath, "utf-8");

let tag = file.substring(script.startTagPosition, script.position);
let tagLines = tag.split("\n");
let leadingLines = file.substring(0, script.startTagPosition).split("\n");
let start = {
line: leadingLines.length,
column: columnsInLastLine(leadingLines),
};

return Promise.reject({
message: BrsError.format(
`Invalid path '${script.attr.uri}' found in <script/> tag`,
{
file: xmlPath,
start: start,
end: {
line: start.line + tagLines.length - 1,
column: start.column + columnsInLastLine(tagLines),
},
}
).trim(),
});
}

if (script.attr) {
componentScripts.push({
type: script.attr.type,
uri: script.attr.uri,
uri: absoluteUri.href,
});
}
});
}

return componentScripts;
}

/**
* Returns the number of columns occupied by the final line in an array of lines as parsed by `xmldoc`.
* xmldoc parses positions to ignore `\n` characters, which is pretty confusing. This function
* compensates for that.
*
* @param lines an array of strings, where each is a line from an XML document
*
* @return the corrected column number for the last line of text as parsed by `xmlDoc`
*/
function columnsInLastLine(lines: string[]): number {
return lines[lines.length - 1].length + lines.length - 1;
}
11 changes: 5 additions & 6 deletions test/componentprocessor/componentprocessor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const fs = require("fs");

const realFs = jest.requireActual("fs");

describe.only("component parsing support", () => {
describe("component parsing support", () => {
afterEach(() => {
fg.sync.mockRestore();
fs.readFile.mockRestore();
Expand Down Expand Up @@ -65,11 +65,10 @@ describe.only("component parsing support", () => {
"scripts/baseComp.brs",
"scripts/extendedComp.brs",
"scripts/utility.brs",
];
].map((f) => path.join(__dirname, "resources", f));
});
fs.readFile.mockImplementation((filename, _, cb) => {
resourcePath = path.join(__dirname, "resources", filename);
realFs.readFile(resourcePath, (err, contents) => {
realFs.readFile(filename, (err, contents) => {
cb(/* no error */ null, contents);
});
});
Expand All @@ -90,7 +89,7 @@ describe.only("component parsing support", () => {
});

it("adds all scripts into node in correct order", async () => {
let map = await getComponentDefinitionMap("/doesnt/matter");
let map = await getComponentDefinitionMap(process.cwd());
let parsedExtendedComp = map.get("ExtendedComponent");
expect(parsedExtendedComp).not.toBeUndefined();
expect(parsedExtendedComp.scripts).not.toBeUndefined();
Expand All @@ -106,7 +105,7 @@ describe.only("component parsing support", () => {

it("ignores extensions of unknown node subtypes", async () => {
fg.sync.mockImplementation(() => {
return ["unknownExtensionComponent.xml"];
return [path.join(__dirname, "resources", "unknownExtensionComponent.xml")];
});

jest.spyOn(global.console, "error").mockImplementation();
Expand Down
2 changes: 1 addition & 1 deletion test/componentprocessor/resources/extendedComponent.xml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8" ?>
<component name="ExtendedComponent" extends="BaseComponent">
<script type="text/brightscript" uri="pkg:/test/componentprocessor/resources/scripts/extendedComponent.brs" />
<script type="text/brightscript" uri="scripts/extendedComponent.brs" />
<script type="text/brightscript" uri="pkg:/test/componentprocessor/resources/scripts/utility.brs" />
<children>
<Label name="label_b" />
Expand Down

0 comments on commit e2cf3a9

Please sign in to comment.