Closed priandsf closed 4 years ago
@runem We work together with the authors of the other LWC related pull request and we integrated their ideas/changes to this one. We also added some documentation to the .md. So we are ready to go, would you mind merging it? There is one remaining this though, that makes the tests failing: https://github.com/runem/web-component-analyzer/issues/181. As of now, there is a work around in the code and some tests disabled. When this will be fixed, or we understand how to fix it, then we'll update the LWC support.
First of all, thanks for this PR, and sorry for the delay in this review!
I just finished the review with only a few points. When these have been fixed this is ready to be merged :+1:
Is it also possible for you to remove the yarn.lock
file for now? I'll rather prefer that this is added in a separate PR, and I would like package-lock.json
to be the source of truth with a Github Action updating yarn.lock
automatically using yarn import
so that they don't get out of sync :-)
Thanks for the review @runem. I made the expected changes. While it will be merged, I'll try it on a real large project. I might find some slight adjustments then. I also found the JS doc issue, which was due to a hyphen in the tag name, which fails the parser with some TS versions.
Thank you @priandsf ! I just added a suggestion to the last outstanding review-comment with a potential path forward.
Thanks @runem - See my comments, I ran the profiler and did not see a potential issue. It also appeared that most of the IO time actually comes from anallyzer-text.ts.
I'll merge this now and publish it as a minor version. I know I'm just being overly cautious, but I'll keep an eye out for any performance regressions even though the changes are probably fine. Afterwards, I'll prioritize a flag so that specific flavors can be loaded manually when running the CLI.
@runem Thanks a lot - I'm going to run it on our own projects!
@runem Are you going to publish a new version to NPM containing these changes?
Unfortunately, I found a couple of regressions when running it against a big set of test projects.
I think the best solution right now is for me to add a flag to the CLI where the flavor can be specified as we talked about.
However, I just published the changes as a beta-version (1.2.0-next.0
) so that you can test it out on your own projects.
Ok, are the regressions due to ambiguities in the component flavor detection? Is there any change I can provide? Anyway, I agree that a flag should make more reliable.
This PR adds support for the LWC web component framework. It provides the baby steps to handle the @api decorator, following the LWC specification (https://lwc.dev/). From an implementation standpoint, it mimics what the LitElement implementation does, but with LWC specific decorators.
The LwcFlavor is not automatically enabled because the @api behavior will break other flavors, see bellow. Currently, the flavor must be set via the "flavors" member of the AnalyzerOption object, which leads to a question: how to enable it from the CLI? Should we add an option to the CLI? @runem
LWC has a custom behavior for component class members: only the members decorated with @api becomes public, and also automatically become HTML attributes. All the other members become protected. This is handled in the 'refineFeature' call, regardless if the component is LWC or not (there is no obvious way to know that). So the flavor has to be enabled on purpose, and only when parsing LWC component. Unless you have a better idea?