feat(compiler): add sourcemap to options and result #696
Conversation
to the compile method. - Add options for disable sourcemaps in compat/minify lwc rollup plugins. - Removes the template/style from the output sourcemap. - javascript transformer returns the sourcemap. - javascript transformer sourcemap defaulted to false. - make rollup bundler of the compiler to return the sourcemaps.
Benchmark resultsBase commit: lwc-engine-benchmark-ie11
|
| @@ -76,6 +78,7 @@ export interface NormalizedStylesheetConfig extends StylesheetConfig { | |||
| export interface NormalizedOutputConfig extends OutputConfig { | |||
| compat: boolean; | |||
| minify: boolean; | |||
| sourcemap: boolean | 'inline'; | |||
There was a problem hiding this comment.
If we only support 'inline' why do we have that option in the first place?
There was a problem hiding this comment.
Actually inline does not make much sense here in the compiler (unless we actually want to add it to the generated .code)
The compiler only return the sourcemap for the specific transformation if this option is true (which by default is false) or inline
how those are bundled together is not responsibility of the compiler.
removing inline option.
| @@ -72,6 +73,8 @@ export async function transformFile( | |||
|
|
|||
| let compatResult; | |||
| if (options.outputConfig.compat) { | |||
| // @todo: Evaluate for removal. | |||
| // **Note: this is dead code since it was only used in the rollup-plugin-lwc, but it was refactored to do this as part of the rollup-plugin-compat | |||
There was a problem hiding this comment.
Yeah we can simplify this dance.
| @@ -56,14 +57,16 @@ export async function bundle( | |||
|
|
|||
| // TODO: remove format option once tests are converted to 'amd' format | |||
| const format = (outputConfig as any).format || DEFAULT_FORMAT; | |||
| const sourcemap = outputConfig.sourcemap; | |||
There was a problem hiding this comment.
Use destructuring
const { sourmap } = outputConfig;| @@ -0,0 +1,23 @@ | |||
| import lwcCompatFactory from '../compat'; | |||
|
|
|||
| const codeFixture = "const a = 1;\nconsole.log(a);"; | |||
There was a problem hiding this comment.
Can you use template string instead of multiline +
| @@ -0,0 +1,20 @@ | |||
| import lwcMinifierFactory from '../minify'; | |||
|
|
|||
| const codeFixture = "/*some comment*/var a = 1;\nconsole.log(a);"; | |||
| /** | ||
| * Rollup plugin applying minification to the generated bundle. | ||
| */ | ||
| export default function() { | ||
| export default function(options?: LwcMinifierOptions) { |
There was a problem hiding this comment.
Let's pass the NormalizedOutputConfig instead of a custom config.
| @@ -13,7 +13,9 @@ export default function( | |||
| options: NormalizedCompilerOptions, | |||
| metadataCollector?: MetadataCollector | |||
| ): FileTransformerResult { | |||
| const config = Object.assign({}, BABEL_CONFIG_BASE, { | |||
|
|
|||
| const overrides = { sourceMaps: options.outputConfig.sourcemap }; | |||
There was a problem hiding this comment.
Add the override in the object literal instead of a new object.
| @@ -1,4 +1,5 @@ | |||
| import { bundle } from '../bundler'; | |||
| import { normalizeOptions } from '../../compiler/options'; | |||
There was a problem hiding this comment.
Don't invoke the normalizeOptions here, if you want to normlize the options invoke compile.
There was a problem hiding this comment.
Please ensure SourceMap type support is added to lwc-compiler as well: https://git.soma.salesforce.com/lwc/lwc-platform/blob/master/lwc/src/main/java/org/lwc/OutputConfig.java
| @@ -15,11 +15,12 @@ import { | |||
|
|
|||
| import { collectImportLocations } from "./import-location-collector"; | |||
| import { Diagnostic, DiagnosticLevel } from "../diagnostics/diagnostic"; | |||
| import {SourceMap} from "../compiler/compiler"; | |||
There was a problem hiding this comment.
pls add spaces between { SourceMap }
| }), | ||
| ).rejects.toMatchObject({ | ||
| message: | ||
| "Expected a boolean or string 'inline' for outputConfig.sourcemap, received \"true\".", |
There was a problem hiding this comment.
this message will need to be changed once you remove 'inline'
from dval, pm and alex.
this is dead code since it was only used in the rollup-plugin-lwc, but it was refactored to do this as part of the rollup-plugin-compat
option when instantiating the rollup-plugin-lwc-compiler
to avoid misleading sourcemaps
Benchmark resultsBase commit: |
generated sourcemap
| const { DEFAULT_OPTIONS, DEFAULT_MODE } = require("./constants"); | ||
|
|
||
| module.exports = function rollupLwcCompiler(pluginOptions = {}) { | ||
| const { include, exclude } = pluginOptions; | ||
| const filter = pluginUtils.createFilter(include, exclude); | ||
| const mergedPluginOptions = Object.assign({}, DEFAULT_OPTIONS, pluginOptions); | ||
| const { resolveFromPackages, resolveFromSource } = mergedPluginOptions; | ||
| const { sourcemap = false } = pluginOptions; |
There was a problem hiding this comment.
sourcemap default value should be added to the DEFAULT_OPTIONS instead of here.
| } | ||
|
|
||
| return result; | ||
| return await transformer(src, id, options, metadataCollector); |
There was a problem hiding this comment.
Remove unnecessary async and await.
There was a problem hiding this comment.
There was a problem hiding this comment.
return await is equivalent to return. So the await is not needed.
If you don't have an await in the function body you don't need the async either.
|
|
||
| // @ts-ignore | ||
| await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => { | ||
| let gp; |
There was a problem hiding this comment.
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 | ||
| await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => { | ||
|
|
||
| let gp; |
There was a problem hiding this comment.
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.
| let gp; | ||
|
|
||
| // @ts-ignore | ||
| gp = sourceMapConsumer.allGeneratedPositionsFor({ line: 2, source: "unknown" }); |
There was a problem hiding this comment.
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.
| metadata: BundleMetadata; | ||
| outputConfig: NormalizedOutputConfig; | ||
| } | ||
|
|
||
| export interface SourceMap { |
There was a problem hiding this comment.
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 ?
| } catch (e) { | ||
| // populate diagnostics | ||
| const { message, filename } = e; | ||
| map = null; |
There was a problem hiding this comment.
we can initialize map to null on line 96 instead.
| let gp; | ||
|
|
||
| // m in main from utils; | ||
| gp = sourceMapConsumer.generatedPositionFor({ |
There was a problem hiding this comment.
we can move this initialization to line 232
| } | ||
| }); | ||
| // @ts-ignore | ||
| await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => { |
There was a problem hiding this comment.
please add expect.assertions count at the top of the test to ensure all the validations take place after promise resolves.
| expect(result.map).not.toBeNull(); | ||
|
|
||
| // @ts-ignore | ||
| await SourceMapConsumer.with(result!.map as RawSourceMap, null, sourceMapConsumer => { |
There was a problem hiding this comment.
pls add expect.assertions into this test as well
jodarove
force-pushed
the
jodarove/compiler-sourcemaps
branch
from
7ab06fa to
c077554
Compare
jodarove
force-pushed
the
jodarove/compiler-sourcemaps
branch
from
c077554 to
b130763
Compare
as a type
to the asyn method.
jodarove
force-pushed
the
jodarove/compiler-sourcemaps
branch
from
1a7f3f3 to
3bf8fb9
Compare
|
|
||
| export interface BundleReport { | ||
| code: string; | ||
| diagnostics: Diagnostic[]; | ||
| map: SourceMap | null; | ||
| map: any | null; |
There was a problem hiding this comment.
i don't think we need null if the type is any
| @@ -15,20 +15,11 @@ export interface CompilerOutput { | |||
|
|
|||
| export interface BundleResult { | |||
| code: string; | |||
| map: SourceMap | null; | |||
| map: any | null; | |||
| @@ -22,7 +21,7 @@ export interface FileTransformerResult { | |||
| metadata?: | |||
| | TemplateMetadata | |||
| | lwcClassTransformPlugin.Metadata; | |||
| map: SourceMap | null; | |||
| map: any | null; | |||
Benchmark resultsBase commit: |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
of the compile method.
Details
This PR adds a sourcemap property to the
OutputConfigof the compiler options. Whensourcemap=truethecompilemethod will return aSourceMapin themapproperty of the result,nullotherwise.Does this PR introduce a breaking change?