fix(compiler): invalid metadata used in decorator index #88
Conversation
Benchmark comparisonBase commit:
|
There was a problem hiding this comment.
The metadata merge fix looks good; however, i'm concerned with not handling metadata for multiple classes - in the event of component class not being processed last, we may endup without metadata, which will cause issues.
We definitely need your metadata merge fix and as for the multiple classes, we need to open an issue.
| @@ -420,7 +420,16 @@ describe('metadata', () => { | |||
| declarationLoc: { | |||
| end: { column: 1, line: 13 }, | |||
| start: { column: 0, line: 3 } | |||
| } | |||
| }, | |||
| marked: [], | |||
| }, | ||
| marked: [], | ||
| modules: { | ||
| exports: { exported: ['Foo'], specifiers: [{ exported: "default", kind: "local", local: "Foo" }] }, |
There was a problem hiding this comment.
What is kind and local used for ?
There was a problem hiding this comment.
kind is the type of export, and local is the local binding when type is local. this sounds a lot like the metadata of modules provided out of the box from babel/rollup.
| { imported: ['Element'], source: 'engine', specifiers: [{ imported: 'Element', kind: 'named', local: 'Element' }] } | ||
| ] | ||
| }, | ||
| usedHelpers: [] |
| modules: { | ||
| exports: { exported: ['Foo'], specifiers: [{ exported: "default", kind: "local", local: "Foo" }] }, | ||
| imports: [ | ||
| { imported: ['api'], source: 'engine', specifiers: [{ imported: 'api', kind: 'named', local: 'api' }] }, |
There was a problem hiding this comment.
Why do you keep track of imported and specifiers independently?
| modules: { | ||
| exports: { exported: ['Foo'], specifiers: [{ exported: "default", kind: "local", local: "Foo" }] }, | ||
| imports: [ | ||
| { imported: ['api'], source: 'engine', specifiers: [{ imported: 'api', kind: 'named', local: 'api' }] }, |
There was a problem hiding this comment.
As discussed this morning with @yungcheng and @apapko, we will stop using state.file.metadata to store the metadata:
- it's a private / not documented API from babel and it will probably break in the future.
- mutating this object can have unexpected side effects on other plugins.
We have agreed that we will rely on plugin config to communicate the metadata to the plugin consumer.
import { transform } from 'babel-core';
import raptorClassTransformPlugin from 'babel-plugin-transform-lwc-class';
const metadata = {};
const { code } = transform(code, {
plugins: [
[
raptorClassTransformPlugin,
{
metadata,
},
],
],
});
// The metadata object get populated by the plugin.
console.log(code, metadata)FYI @rsalvador.
yungcheng
force-pushed
the
vince/decorator-metadata-fix
branch
from
1b18bef
to
c5338ec
Compare
Benchmark comparisonBase commit:
|
Benchmark comparisonBase commit:
|
| @@ -29,16 +29,19 @@ module.exports = function ({ types: t }) { | |||
| const classBody = path.get('body'); | |||
|
|
|||
| if (isDefaultExport(path)) { | |||
| const hasMetadata = this.opts && this.opts.hasOwnProperty('metadata'); | |||
| const declaration = path.parentPath.node; | |||
| if (declaration.leadingComments) { | |||
| const lastComment = declaration.leadingComments[declaration.leadingComments.length - 1].value; | |||
There was a problem hiding this comment.
can declaration leading comments be 0 length? Just curios if we can hit out of bound index
| @@ -140,7 +141,9 @@ module.exports = function decoratorVisitor({ types: t }) { | |||
| // Note: In the (extremely rare) case of multiple classes in the same file, only the metadata about the | |||
| // last class will be returned | |||
| const metadata = transform(t, klass, decorators); | |||
| state.file.metadata = Object.assign({}, state.metadata, metadata); | |||
| if (hasMetadata) { | |||
There was a problem hiding this comment.
Is it safe to assume that if metadata has been provided during plugins initialization ( line 11 of javascript.js ), then it will always appear in this.opts?
| @@ -140,7 +141,9 @@ module.exports = function decoratorVisitor({ types: t }) { | |||
| // Note: In the (extremely rare) case of multiple classes in the same file, only the metadata about the | |||
| // last class will be returned | |||
| const metadata = transform(t, klass, decorators); | |||
| state.file.metadata = Object.assign({}, state.metadata, metadata); | |||
| if (hasMetadata) { | |||
| this.opts.metadata = Object.assign({}, this.opts.metadata, metadata); | |||
There was a problem hiding this comment.
since we have better control of metadata format here, should we attempt to map metadata to a classname? Or have a list of metadata objects - one per classname? I'm just thinking about your comment above, it could bite us
| const declaration = path.parentPath.node; | ||
| if (declaration.leadingComments) { | ||
| const lastComment = declaration.leadingComments[declaration.leadingComments.length - 1].value; | ||
| const sanitized = sanitizeComment(lastComment); | ||
| if (sanitized) { | ||
| state.file.metadata.doc = sanitized; | ||
| if (sanitized && hasMetadata) { |
There was a problem hiding this comment.
sanitizeComment function declaration needs param existence check prior invoking trim()
| @@ -19,6 +19,6 @@ export default function(code, options) { | |||
| return { | |||
| code: transformed.code, | |||
| map: transformed.map, | |||
| metadata: transformed.metadata, | |||
| metadata: transformed.options.plugins[0][1].metadata | |||
There was a problem hiding this comment.
is plugins order always preserved?
|
After going through the babel issues, it appears that they want to enforce aggressive caching for plugin config (babel/babel#6326, babel/babel#6350). Even if I dislike it, using the Before merging the PR, we should also create an issue to fix that in the future. |
yungcheng
force-pushed
the
vince/decorator-metadata-fix
branch
from
b5b5baf
to
24769fe
Compare
Benchmark comparisonBase commit:
|
There was a problem hiding this comment.
We don't have a better solution for now in terms of surfacing the metadata.
@yungcheng LGTM, to unblock us for now. Can you open to track the cleanup work that we need to do?
Details
Addressing discussion here I believe it should've been
state.file.metadataDoes this PR introduce a breaking change?