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(rsg): Support relative paths in <script/> tags #474

Merged
merged 3 commits into from
Jul 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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