-
Notifications
You must be signed in to change notification settings - Fork 400
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): introduce compile/transform/bundle diagnostics #256
Conversation
* wip: module import locations * wip: move import locations into metadata * wip: rename import locations * wip: use common location class in import locations * wip: change regex and token retrieval
Benchmark comparisonBase commit:
|
Benchmark comparisonBase commit:
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
* feat(compiler): use real compiler version during build
Benchmark resultsBase commit: lwc-engine-benchmark
|
package.json
Outdated
] | ||
], | ||
"dependencies": { | ||
"source-map": "^0.7.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove root level dependency
packages/lwc-compiler/package.json
Outdated
"@types/babel-core": "^6.25.3", | ||
"@types/chokidar": "^1.7.5", | ||
"magic-string": "^0.22.4", | ||
"source-map": "^0.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it used?
packages/lwc-compiler/package.json
Outdated
@@ -29,6 +30,10 @@ | |||
"rollup-plugin-replace": "^2.0.0" | |||
}, | |||
"devDependencies": { | |||
"@types/babel-core": "^6.25.3", | |||
"@types/chokidar": "^1.7.5", | |||
"magic-string": "^0.22.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it used?
packages/lwc-compiler/package.json
Outdated
@@ -29,6 +30,10 @@ | |||
"rollup-plugin-replace": "^2.0.0" | |||
}, | |||
"devDependencies": { | |||
"@types/babel-core": "^6.25.3", | |||
"@types/chokidar": "^1.7.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it used?
const { version } = require("../package.json"); | ||
|
||
async function updateVersion(version) { | ||
const inputPath = (outputPath = path.resolve("dist/commonjs/index.js")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No single line assignment. This will not work if this file is in strict mode.
// split result: ["'x-bar', 'x-foo'"] | ||
const imports = rawImports.split(/,\s*/) || []; | ||
|
||
imports.forEach(moduleImport => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good old Array.prototype.map
would do the trick here instead of a Array.prototype.forEach
. This way the callback can become pure.
packages/lwc-compiler/src/options.ts
Outdated
files: BundleFiles; | ||
outputConfig?: OutputConfig; | ||
|
||
// TODO: below must be removed after lwc-compiler consumers change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needed?
}); | ||
const result = babel.transform(code, config); | ||
|
||
if (!result || !result.code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is it the case?
I would like to have a more explicit error when babel doesn't return any code.
src: string, | ||
filename: string, | ||
{ outputConfig }: NormalizedCompilerOptions, | ||
metadataCollector?: MetadataCollector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused param
src: string, | ||
filename: string, | ||
options: NormalizedCompilerOptions, | ||
metadataCollector?: MetadataCollector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove used param
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
||
describe("compiler options", () => { | ||
it("should validate presence of options", async () => { | ||
expect.assertions(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the expect.assertion(1)
. Not needed anymore.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Details
This PR includes several enhancements to the open source compiler to allow for source code diagnostics and cleaner integration with platform compiler. The open source compiler will be consumed by platform-compiler for transformation and bundling invocation. Furthermore, as part of the effort to minimize compiler maintenance and code/feature duplication, platform-compiler, which in-itself consumes open source compiler, will be now used by both Aura open source and core in the form of a jar ( maven dependency ).
Some details on the changes:
Few remaining TODOS:
Does this PR introduce a breaking change?
Compiler interface has changed. Aura will now be using platform compiler instead - thus invocation API must be change in Aura to satisfy platform compiler interface - tracked separately.