Skip to content

Commit

Permalink
Change NetworkData to contain Request and Response
Browse files Browse the repository at this point in the history
Move members of `NetworkData` interface into more specialized
interfaces (i.e. `Request` and `Response`), and add those more
specialised interfaces as members of `NetworkData`.

This change is done in order to remove the need to work around
potential naming conflicts (i.e. both the `request` and `response`
have `headers`), and thus, easily allow future additions (e.g.:
adding information about network timing, network security, etc.),
without requiring any changes.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref: #54 (comment)
  • Loading branch information
alrra committed Mar 22, 2017
1 parent 162cc6b commit ba04893
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 66 deletions.
26 changes: 16 additions & 10 deletions src/lib/collectors/jsdom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,13 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
const body = await readFileAsync(targetPath);

return {
body,
headers: null,
originalBody: null,
statusCode: null
request: { headers: null },
response: {
body,
headers: null,
originalBody: null,
statusCode: null
}
};
};

Expand All @@ -98,10 +101,13 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
const [response, body] = await req(href);

return {
body,
headers: response.headers,
statusCode: response.statusCode
// Add original compressed bytes here (originalBytes)
request: { headers: response.request.headers },
response: {
body,
headers: response.headers,
originalBody: null, // Add original compressed bytes here (originalBytes)
statusCode: response.statusCode
}
};
};

Expand Down Expand Up @@ -176,7 +182,7 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
return;
}

const { headers: responseHeaders, body: html } = contentResult;
const { response: { headers: responseHeaders, body: html } } = contentResult;

// making this data available to the outside world
_headers = responseHeaders;
Expand Down Expand Up @@ -227,7 +233,7 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {
await server.emitAsync('fetch::start', resourceUrl);

try {
const { body: resourceBody, headers: resourceHeaders } = await _fetchContent(resourceUrl);
const { response: { body: resourceBody, headers: resourceHeaders } } = await _fetchContent(resourceUrl);

debug(`resource ${resourceUrl} fetched`);

Expand Down
2 changes: 1 addition & 1 deletion src/lib/rules/manifest-exists/manifest-exists.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const rule: RuleBuilder = {
// exists and is accesible.

try {
const { statusCode } = await context.fetchContent(manifestURL);
const { response: { statusCode } } = await context.fetchContent(manifestURL);

// If it's not a local file (has statusCode === null),
// check also if the status code is `200`.
Expand Down
67 changes: 39 additions & 28 deletions src/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ export interface RuleBuilder {
/** The metadata associated to the rule (docs, schema, etc.) */
meta: {
/** Documentation related to the rule */
docs?: any,
docs?: any;
/** If this rule can autofix the issue or not */
fixable?: string,
fixable?: string;
/** The schema the rule configuration must follow in order to be valid */
schema: Array<any>
schema: Array<any>;
};
}

Expand All @@ -22,7 +22,7 @@ export interface Rule {

/** The builder of a Collector */
export interface CollectorBuilder {
(sonar, options): Collector,
(sonar, options): Collector;
}

/** A collector to be used by Sonar */
Expand All @@ -39,21 +39,32 @@ export interface Collector {
fetchContent(target: URL | string, customHeaders?: object): Promise<NetworkData>;
}

/** The response of fetching an item using the request of a collector */
export interface NetworkData {
/** Request data from fetching an item using a collector */
export interface Request {
headers: object;
}

/** Response data from fetching an item using a collector */
export interface Response {
body: string;
headers: object;
originalBytes?: Uint8Array, // TODO: is this the right type?
statusCode: number
originalBytes?: Uint8Array; // TODO: is this the right type?
statusCode: number;
}

/** Network data from fetching an item using a collector */
export interface NetworkData {
response: Response;
request: Request;
}

export interface Config {
sonarConfig?
sonarConfig?;
}

/** A format function that will output the results obtained by Sonar */
export interface Formatter {
format(problems: Array<Problem>): void
format(problems: Array<Problem>): void;
}

/** A specialized builder of plugins to be used by Sonar */
Expand All @@ -65,7 +76,7 @@ export interface PluginBuilder {
/** A plugin that expands the collector's functionality */
export interface Plugin {
// TBD
any
any;
}

/** A resource required by Sonar: Collector, Formatter, Plugin, Rule, */
Expand All @@ -76,27 +87,27 @@ export type URL = url.Url; // eslint-disable-line no-unused-vars

export interface Page {
/** The document of page */
dom: HTMLElement,
dom: HTMLElement;
/** The original HTML string of the resource */
html: string,
html: string;
/** The response headers obtained when requesting the page */
responseHeaders?: object
responseHeaders?: object;
}

/** A problem found by a Rule in Sonar */
export interface Problem {
/** The column number where the Problem is */
column: number,
column: number;
/** The line number where the Problem is */
line: number,
line: number;
/** A message providing more information about the Problem */
message: string,
message: string;
/** The uri of the resource firing this event */
resource: string,
resource: string;
/** The name of the triggered rule */
ruleId: string,
ruleId: string;
/** The severity of the rule based on the actual configuration */
severity: Severity
severity: Severity;
}

/** The severity configuration of a Rule */
Expand All @@ -109,27 +120,27 @@ export enum Severity {
/** The location of a Problem in the code */
export interface ProblemLocation {
/** The column number where a Problem is */
column: number,
column: number;
/** The line number where a Problem is */
line: number
line: number;
}

/** The object emited by a collector on `fetch::end` */
export interface FetchEndEvent {
/** The uri of the resource firing this event */
resource: string,
resource: string;
/** The HTMLElement that started the fetch */
element: HTMLElement,
element: HTMLElement;
/** The content of target in the url or href of element */
content: string,
content: string;
/** The headers of the response */
headers: object
headers: object;
}

/** The object emited by a collector on `element::TYPEOFELEMENT` */
export interface ElementFoundEvent {
/** The uri of the resource firing this event */
resource: string,
resource: string;
/** The HTMLElement found */
element: HTMLElement
element: HTMLElement;
}
6 changes: 3 additions & 3 deletions src/tests/helpers/rule-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ const runRule = async (t: ContextualTestContext, ruleTest: RuleTest) => {
.slice(0, 2)
.join('::');

if (event.responses) {
if (event.networkData) {
ruleContext.fetchContent = sinon.stub();

event.responses.forEach((response, i) => { // eslint-disable-line no-loop-func
ruleContext.fetchContent.onCall(i).returns(Promise.resolve(response));
event.networkData.forEach((data, i) => { // eslint-disable-line no-loop-func
ruleContext.fetchContent.onCall(i).returns(Promise.resolve(data));
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/tests/helpers/rule-test-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export interface TestEvent {
name: string,
/** The path to the fixture to use when sending the event */
fixture: string,
/** The response data that should be returned */
responses?: Array<NetworkData>
/** The network data (i.e. request, response data) that should be returned */
networkData?: Array<NetworkData>
}

export interface Report {
Expand Down
4 changes: 2 additions & 2 deletions src/tests/lib/collectors/jsdom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ test(async (t) => {
const filePath = fileUrl(path.resolve(__dirname, './fixtures/file-protocol.txt'));
const content = await collector.fetchContent(filePath);

t.is(content.body, 'This is a file read using file://', 'jsdom collector can read file://');
t.falsy(content.headers, 'no headers are returned for file:// target');
t.is(content.response.body, 'This is a file read using file://', 'jsdom collector can read file://');
t.falsy(content.response.headers, 'no headers are returned for file:// target');
});
55 changes: 35 additions & 20 deletions src/tests/lib/rules/manifest-exists/tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@ const tests: Array<RuleTest> = [
events: [{
name: 'element::link::0',
fixture: path.resolve(__dirname, './fixtures/manifest-specified-multiple-times.html'),
responses: [{
body: null,
headers: null,
statusCode: 200
networkData: [{
request: { headers: null },
response: {
body: null,
headers: null,
statusCode: 200
}
}]
}, {
name: 'element::link::1',
Expand Down Expand Up @@ -60,10 +63,13 @@ const tests: Array<RuleTest> = [
events: [{
name: 'element::link',
fixture: path.resolve(__dirname, './fixtures/manifest-specified-as-full-url.html'),
responses: [{
body: null,
headers: null,
statusCode: 200
networkData: [{
request: { headers: null },
response: {
body: null,
headers: null,
statusCode: 200
}
}]
}]
},
Expand All @@ -72,10 +78,13 @@ const tests: Array<RuleTest> = [
events: [{
name: 'element::link',
fixture: path.resolve(__dirname, './fixtures/manifest-specified-and-file-does-exist.html'),
responses: [{
body: null,
headers: null,
statusCode: 200
networkData: [{
request: { headers: null },
response: {
body: null,
headers: null,
statusCode: 200
}
}]
}]
},
Expand All @@ -92,10 +101,13 @@ const tests: Array<RuleTest> = [
events: [{
name: 'element::link',
fixture: path.resolve(__dirname, './fixtures/manifest-specified-and-file-does-exist.html'),
responses: [{
body: null,
headers: null,
statusCode: 404
networkData: [{
request: { headers: null },
response: {
body: null,
headers: null,
statusCode: 404
}
}]
}],
report: { message: `Web app manifest file could not be fetched (status code: 404)` }
Expand All @@ -105,10 +117,13 @@ const tests: Array<RuleTest> = [
events: [{
name: 'element::link',
fixture: path.resolve(__dirname, './fixtures/manifest-specified-and-file-does-exist.html'),
responses: [{
body: null,
headers: null,
statusCode: 500
networkData: [{
request: { headers: null },
response: {
body: null,
headers: null,
statusCode: 500
}
}]
}],
report: { message: `Web app manifest file could not be fetched (status code: 500)` }
Expand Down

0 comments on commit ba04893

Please sign in to comment.