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

feat(compiler): add sourcemap to options and result #696

Merged
merged 12 commits into from
Oct 4, 2018
1 change: 1 addition & 0 deletions packages/lwc-compiler/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@types/babel-core": "~6.25.3",
"awesome-typescript-loader": "^5.0.0",
"magic-string": "^0.22.4",
"source-map": "^0.7.3",
"string-replace-webpack-plugin": "^0.1.3",
"webpack": "^4.10.2",
"webpack-cli": "^3.0.2"
Expand Down
50 changes: 50 additions & 0 deletions packages/lwc-compiler/src/bundler/__tests__/bundler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,54 @@ describe('bundler', () => {
),
});
});

test('does not return sourcemap by default', async () => {
const result = await bundle({
baseDir: '',
name: 'cmp',
namespace: 'c',
files: {
'cmp.js': 'let a = 1'
},
stylesheetConfig: {
customProperties: {
allowDefinition: false,
resolution: { type: 'native' }
}
},
outputConfig: {
env: { NODE_ENV: 'development' },
minify: false,
compat: false,
sourcemap: false
}
});

expect(result.map).toBeNull();
});

test('returns sourcemap when requested', async () => {
const result = await bundle({
baseDir: '',
name: 'cmp',
namespace: 'c',
files: {
'cmp.js': 'const a = 1;'
},
stylesheetConfig: {
customProperties: {
allowDefinition: false,
resolution: { type: 'native' }
}
},
outputConfig: {
env: { NODE_ENV: 'development' },
minify: false,
compat: false,
sourcemap: true
}
});

expect(result.map).not.toBeNull();
});
});
14 changes: 9 additions & 5 deletions packages/lwc-compiler/src/bundler/bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import {

import { collectImportLocations } from "./import-location-collector";
import { Diagnostic, DiagnosticLevel } from "../diagnostics/diagnostic";
import { SourceMap } from "../compiler/compiler";
jodarove marked this conversation as resolved.
Show resolved Hide resolved

export interface BundleReport {
code: string;
diagnostics: Diagnostic[];
map: null;
map: SourceMap | null;
metadata: BundleMetadata;
}

Expand Down Expand Up @@ -84,14 +85,15 @@ export async function bundle(
);

if (outputConfig.compat) {
plugins.push(rollupCompat());
plugins.push(rollupCompat(outputConfig));
}

if (outputConfig.minify) {
plugins.push(rollupMinify());
plugins.push(rollupMinify(outputConfig));
}

let code;
let map;
try {
const rollupBundler = await rollup({
input: name,
Expand All @@ -103,13 +105,15 @@ export async function bundle(
amd: { id: namespace + "/" + name },
interop: false,
strict: false,
sourcemap: outputConfig.sourcemap,
format
});
code = result.code;

map = result.map;
} catch (e) {
// populate diagnostics
const { message, filename } = e;
map = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can initialize map to null on line 96 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, fixing it


diagnostics.push({
filename,
Expand All @@ -125,7 +129,7 @@ export async function bundle(
return {
diagnostics,
code,
map: null,
map,
pmdartus marked this conversation as resolved.
Show resolved Hide resolved
metadata: metadataCollector.getMetadata()
};
}
16 changes: 16 additions & 0 deletions packages/lwc-compiler/src/compiler/__tests__/options.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ describe('compiler options', () => {
});
});

it('should validate outputConfig.sourcemap', async () => {
await expect(
compile({
name: 'foo',
namespace: 'x',
files: { x: 'foo' },
outputConfig: {
sourcemap: 'true',
},
}),
).rejects.toMatchObject({
message:
"Expected a boolean value for outputConfig.sourcemap, received \"true\".",
});
});

it('should validate stylesheetConfig.customProperties.allowDefinition', async () => {
await expect(
compile({
Expand Down
72 changes: 72 additions & 0 deletions packages/lwc-compiler/src/compiler/__tests__/result.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { compile } from "../compiler";
import { pretify, readFixture } from "../../__tests__/utils";
import { DiagnosticLevel } from "../../diagnostics/diagnostic";
import { RawSourceMap, SourceMapConsumer } from "source-map";

const VALID_CONFIG = {
outputConfig: {
Expand Down Expand Up @@ -200,6 +201,77 @@ describe("compiler result", () => {
expect(diagnostics[1].level).toBe(DiagnosticLevel.Fatal);
expect(diagnostics[1].message).toContain('foo.html: <template> has no matching closing tag.');
});

test("sourcemaps correctness", async () => {
const cmpCode = `import { LightningElement } from 'lwc';
import { main } from './utils/util.js';
jodarove marked this conversation as resolved.
Show resolved Hide resolved
export default class Test extends LightningElement {
get myimport() {
return main();
}
}
`;
const utilsCode = `export function main() {
return 'here is your import';
}`;
const { result } = await compile({
name: "foo",
namespace: "x",
files: {
"foo.js": cmpCode,
"foo.html": readFixture("metadata/metadata.html"),
"utils/util.js": utilsCode,
},
outputConfig: {
sourcemap: true
}
});
// @ts-ignore
await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add expect.assertions count at the top of the test to ensure all the validations take place after promise resolves.


let gp;

// m in main from utils;
gp = sourceMapConsumer.generatedPositionFor({
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can move this initialization to line 232

source: 'utils/util.js',
line: 1,
column: 16,
});

expect(gp.line).toBe(26);
expect(gp.column).toBe(13);

// ' in return 'here ....
gp = sourceMapConsumer.generatedPositionFor({
source: 'utils/util.js',
line: 2,
column: 9,
});

expect(gp.line).toBe(27);
expect(gp.column).toBe(13);

// m in myimport()
gp = sourceMapConsumer.generatedPositionFor({
source: 'foo.js',
line: 4,
column: 6,
});

expect(gp.line).toBe(31);
expect(gp.column).toBe(10);

// m in main()
gp = sourceMapConsumer.generatedPositionFor({
source: 'foo.js',
line: 5,
column: 11,
});

expect(gp.line).toBe(32);
expect(gp.column).toBe(15);
});
});
});

describe("compiler metadata", () => {
Expand Down
14 changes: 12 additions & 2 deletions packages/lwc-compiler/src/compiler/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,20 @@ export interface CompilerOutput {

export interface BundleResult {
code: string;
map: null;
map: SourceMap | null;
metadata: BundleMetadata;
outputConfig: NormalizedOutputConfig;
}

export interface SourceMap {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it as any. Each tool as a different slightly different format for the sourcemap, and since we are not in the business of manipulating the source map in the compiler directly, I feel more confident to put it as any.

type SourceMap = any;

Thoughts @jodarove ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree

version?: string;
file?: string;
sourceRoot?: string;
sources?: string[];
names?: string[];
mappings: string;
}

export async function compile(
options: CompilerOptions
): Promise<CompilerOutput> {
Expand All @@ -32,6 +41,7 @@ export async function compile(
const {
diagnostics: bundleDiagnostics,
code,
map,
metadata,
} = await bundle(normalizedOptions);

Expand All @@ -40,7 +50,7 @@ export async function compile(
if (!hasError(diagnostics)) {
result = {
code,
map: null,
map,
metadata,
outputConfig: normalizedOptions.outputConfig,
};
Expand Down
12 changes: 11 additions & 1 deletion packages/lwc-compiler/src/compiler/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ const DEFAULT_STYLESHEET_CONFIG: NormalizedStylesheetConfig = {
const DEFAULT_OUTPUT_CONFIG: NormalizedOutputConfig = {
env: {},
minify: false,
compat: false
compat: false,
sourcemap: false
};

const KNOWN_ENV = new Set([
Expand All @@ -37,6 +38,7 @@ export interface StylesheetConfig {
export interface OutputConfig {
compat?: boolean;
minify?: boolean;
sourcemap?: boolean;
env?: {
NODE_ENV?: string;
};
Expand Down Expand Up @@ -74,6 +76,7 @@ export interface NormalizedStylesheetConfig extends StylesheetConfig {
export interface NormalizedOutputConfig extends OutputConfig {
compat: boolean;
minify: boolean;
sourcemap: boolean;
env: {
NODE_ENV?: string;
};
Expand Down Expand Up @@ -171,6 +174,13 @@ function validateOutputConfig(config: OutputConfig) {
);
}

if (!isUndefined(config.sourcemap) && !isBoolean(config.sourcemap)) {
throw new TypeError(
`Expected a boolean value for outputConfig.sourcemap, received "${
config.sourcemap
}".`
);
}
if (!isUndefined(config.env)) {
if (!isObject(config.env)) {
throw new TypeError(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import lwcCompatFactory from '../compat';
import { NormalizedOutputConfig } from "../../compiler/options";
import { RawSourceMap, SourceMapConsumer } from "source-map";
Copy link
Member

Choose a reason for hiding this comment

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

Add module imports before relative imports.


const codeFixture = `
const a = 1;
console.log(a);
`;
const compatCode = `import __callKey1 from "proxy-compat/callKey1";
var a = 1;

__callKey1(console, "log", a);`;

describe('rollup plugin lwc-compat', () => {
test('output without sourcemap', () => {
const lwcCompat = lwcCompatFactory({ sourcemap: false } as NormalizedOutputConfig);
const result = lwcCompat.transform(codeFixture);

expect(result.code).toBe(compatCode);
expect(result.map).toBeNull();
});
test('outputs a correct sourcemap', async () => {
const lwcCompat = lwcCompatFactory({ sourcemap: true } as NormalizedOutputConfig);
const result = lwcCompat.transform(codeFixture);

expect(result.map).not.toBeNull();

// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why ignore?

await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls add expect.assertions into this test as well


let gp;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of reusing the gp create new variable: commentPosition, varPosition, ...
This would also remove the need to have comments in your code explaining what you are looking at.


// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why ignore?

gp = sourceMapConsumer.allGeneratedPositionsFor({ line: 2, source: "unknown" });
Copy link
Member

Choose a reason for hiding this comment

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

Why the source is unkown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause no sourceFileName option is passed to the babel transform method (https://babeljs.io/docs/en/options#sourcefilename) unknown is the default.

Since this is used in a rollup plugin, it does not look into source property, only to the mappings one.


// const -> var;
expect(gp[0].line).toBe(2);
expect(gp[0].column).toBe(0);
expect(gp[0].lastColumn).toBe(3);

// a -> a; *Note: gp[1] is the semicolon mapping.
expect(gp[2].line).toBe(2);
expect(gp[2].column).toBe(4);
expect(gp[2].lastColumn).toBe(4);

// mappings for the console.log
// @ts-ignore
gp = sourceMapConsumer.allGeneratedPositionsFor({ line: 3, source: "unknown" });

// console -> __callKey1(console
expect(gp[0].line).toBe(4);
expect(gp[0].column).toBe(0);
expect(gp[0].lastColumn).toBe(10);

});
});
});
Loading