open-wc / custom-elements-manifest

Custom Elements Manifest is a file format that describes custom elements in your project.
https://custom-elements-manifest.open-wc.org/
225 stars 37 forks source link

Using a dependancy is not working correctly #205

Closed matteematt closed 1 year ago

matteematt commented 1 year ago

Checklist

Completing the items above will greatly improve triaging time of your issue.

Expected behavior A clear and concise description of what you expected to happen.

I have a project which some directories like the following

node_modules/@pkg/pkg/dist/custom-elements.json
web/src/

The web/src directory contains ts files with custom elements. I want to create a manifest out of them, and also include the one that is in my node modules package.

I run $ cem analyze --globs {node_modules/@pkg,web/src}/**/*.{js,ts,json} --fast --dependencies. I get the output of the web/src into a manifest which is great, but it does not include the manifest from the node modules that I want.

If I manually copy the custom-elements.json out of the node modules into the web/src/ directory then it does pick it up, but it doesn't actually add any of it to the json file. In this instance I just get this extra empty block added to the manifest it generates

  "modules": [
    {
      "kind": "javascript-module",
      "path": "web/src/custom-elements.json",
      "declarations": [],
      "exports": []
    },

Am I misunderstanding how this works? Should I be manually just merging the module arrays from both of the json files myself?

matteematt commented 1 year ago

With regards to the first issue, there is something wrong with the path globbing. If I do some debugging to print https://github.com/open-wc/custom-elements-manifest/blob/54c2f086d6fe1520580799d7e967dfacc5645866/packages/analyzer/cli.js#L42 then it isn't picking up manifest from the node modules:

fullPathGlobs [
  '/project/client/web/src/custom-elements.json',
  '/project/client/web/src/index.federated.ts',
  '/project/client/web/src/index.ts',
  '/project/client/web/src/types.ts',
  '/project/client/web/src/components/components.ts',
  '/project/client/web/src/components/index.ts',
  '/project/client/web/src/routes/config.ts',
]

If I change the glob to node_modules/@pkg/**/*.{js,ts,json} then I just get

fullPathGlobs []

Also if I hardcode the path as the glob node_modules/@pkg/pkg/dist/custom-elements.json then I still get no match.

matteematt commented 1 year ago

Ok so I have found the reason for the first issue https://github.com/open-wc/custom-elements-manifest/blob/54c2f086d6fe1520580799d7e967dfacc5645866/packages/analyzer/src/utils/cli-helpers.js#L11

The fact that ignore is ignoring the node modules and is the last thing which is mixed into the merged glob array is blocking my glob. Perhaps we need to disable that glob being added if we have the --dependencies flag? Or if we mixin ...IGNORE first then we can let users override the node modules ignore with their own glob.

I am still observing the same issue from when I was copying the file out manually anyway though - I am only getting this extra empty section in my output manifest

      "kind": "javascript-module",
      "path": "node_modules/@pkg/pkg/dist/custom-elements.json",
      "declarations": [],
      "exports": []
    },
matteematt commented 1 year ago

With my second issue, I think I have tracked it down to an error being thrown on this line https://github.com/open-wc/custom-elements-manifest/blob/54c2f086d6fe1520580799d7e967dfacc5645866/packages/find-dependencies/src/find-dependencies.js#L35

It seems we are using es-module-lexer on the json manifest which is a parse error (it seems to expect source files so I don't understand why we are feeding the json file in) which is throwing an error

matteematt commented 1 year ago

So after seeing that it is expecting source files rather than the json manifest I changed my glob to look at source files in the appropriate node modules directory. It does parse them but it falls over constantly on errors like this [CORE - CLASSES]: Looks like you've hit an error in the core library.

I think that this point with my investigation into the glob pattern done but a bit stuck on this current issue, it makes sense for me to wait for a maintainer to get back to me with thoughts on my findings, and whether there is some user error on my end

thepassle commented 1 year ago

Thanks for your investigation! I'm unable to reproduce this locally. Would it be possible for you to make a minimal reproduction repo I could take a look at?

image
matteematt commented 1 year ago

Thanks for the reply. Before I try to do that, can I clarify what --dependencies is actually doing? Just to stress, analysing my local repo with its unadultarated typescript source files works perfectly. The issue that I am trying to also get the info of the web components from a library in the package.json into the same manifest

  1. Are we able to just supply the generated manifest from the library by itself? That is esentially what I was trying, but it was falling over when trying to parse the imports and exports from the json file
  2. Are we supposed to be feeding it the dist files from the library? I did end up trying that too, but it fell over on loads of different files
  3. Are we supposed to be feeing it the source files from the library? I presume this isn't it, as it would not be possible for us to get the typescript source file from the library
thepassle commented 1 year ago

When you use --dependencies it will try to find manifests of other libraries in your node_modules, and if any components from those libraries are used/extended, it will supplement the data for your current projects manifest.

To clarify, here's a run without --dependencies:

image

You can see the manifest includes the superclass as being from a third party package, but none of the methods inherited by GenericTabs are output in the manifest.

When I use --dependencies, the inherited methods of GenericTabs (and whatever else that inherits, in this case its GenericTabs -> SelectedMixin -> BatchingElement), are also included in the classDoc of MyEl:

image
matteematt commented 1 year ago

Ok to be honest then I don't think that is actually exactly what I want to do. I want to have both manifest files combined so I can just use the data for all of it. For reference I am trying to build an LSP plugin for custom elements which is using data from the manifest to completion and diagnostics. In this case would it be syntaxtically correct for me to just merge the modules arrays from the two files?

Regardless, I still think I do want the dependencies linked because if in the base repo we extend something from the library, we want relevent info about attributes on the elements we extend too. But I think for that if I am understanding you correctly, I don't pass it the glob for the node modules at all - the script should be working out what files we want from the node modules via the imports and exports of the local source files?

thepassle commented 1 year ago

In this case would it be syntaxtically correct for me to just merge the modules arrays from the two files?

I think so, but you may have to adjust/correct paths

But I think for that if I am understanding you correctly, I don't pass it the glob for the node modules at all - the script should be working out what files we want from the node modules via the imports and exports of the local source files?

Yeah, this should be based on the source code and what the source code uses

For reference I am trying to build an LSP plugin for custom elements which is using data from the manifest to completion and diagnostics

Cool, have you seen https://github.com/Matsuuu/custom-elements-language-server ? cc @Matsuuu

matteematt commented 1 year ago

Ok I think this thread is probably user error on my part so apologies from the confusion. I need to investigate those two seperate points myself.

Cool, have you seen https://github.com/Matsuuu/custom-elements-language-server ?

Yes, but I only recently became aware of that after I had already started my investigation - I need to look at that properly. We have taken different approaches though. Mine is a typescript plugin so you'd install it via node and then into the tsconfig.json rather than it being an IDE plugin. For our specific use case that is better I think, but it still needs some thought

thepassle commented 1 year ago

Ok I think this thread is probably user error on my part so apologies from the confusion. I need to investigate those two seperate points myself.

Alright, I'll close this issue then if that's okay with you :)

Yes, but I only recently became aware of that after I had already started my investigation

It happens :) Different approaches are healthy 👍 Maybe there's an opportunity for collaboration, or to learn from eachother. Would love to see the result of your project when its finished!

matteematt commented 1 year ago

Yes I am happy for you to close this issue, thanks for spending the time investigating and explaining.

Also yes, I am going to carry on my proof of concept investigation for now and will hopefully be able to share my findings once I have. Then I can take the decision from there about whether it is worth continuing/using Matsuu's instead/or even collaborating on one together

Matsuuu commented 1 year ago

@matteematt the CELS has a detached core that uses typesript and CEM and doesn't fixate on implementation details.

As for tsconfig setup vs LSP, I prefer the LSP approach but understand that it might not fit your cases but would be open to discuss in github issues or discord

matteematt commented 1 year ago

@Matsuuu sounds good. Github issues might be better for being public. Would I raise an issue on your repo?

Matsuuu commented 1 year ago

@matteematt Sure! Go ahead