Skip to content
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

Add unit tests for analyzer (starting with handleJsDoc) #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tlouisse
Copy link
Contributor

@tlouisse tlouisse commented May 4, 2022

Hi,

When running the tool on our Lion components (LionInputAmount) in particular, we found out that handleJsDoc does not recognize object params built from multiple @param tags.

For instance:

class X extends HTMLElement {
  /**
   * @param {object} opts
   * @param {string} [opts.currency]
   */
  callMe({currency}) {
    return currency + '10.00';
  }
}

Is expected to return the following parameters:

     [
        {
          name: 'opts',
          type: {
            text: '{currency?: string}',
          },
        },
      ]

(currently it returns two params).

I created a failing unit test for the scenario above (it's skipped on purpose now, so this can be merged if wanted).

@thepassle since there were only integration tests for the analyzer package so far, feel free to say if you don't agree with the approach or want to do this in a different way :)

@netlify
Copy link

netlify bot commented May 4, 2022

Deploy Preview for custom-elements-manifest-analyzer ready!

Name Link
🔨 Latest commit 181826d
🔍 Latest deploy log https://app.netlify.com/sites/custom-elements-manifest-analyzer/deploys/62722e72067d3d00089ce7ae
😎 Deploy Preview https://deploy-preview-170--custom-elements-manifest-analyzer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tlouisse tlouisse marked this pull request as ready for review May 4, 2022 08:00
@thepassle
Copy link
Member

Did you also want to actually apply the function anywhere in this PR? Or are you planning that for a separate PR?

@tlouisse
Copy link
Contributor Author

tlouisse commented May 4, 2022

Did you also want to actually apply the function anywhere in this PR? Or are you planning that for a separate PR?

We can work around it atm, so I added .skip for now. But maybe in the future I want to do it in a separate PR :).
@daKmoR and me just had a look, and it should be possible to get all type info from the TS AST.

Screenshot 2022-05-04 at 16 28 58

(Type info is found on the right)

See: https://ts-ast-viewer.com/#code/MYGwhgzhAEAa0FMAeAXBA7AJjAEgFQFkAZAURAQFsMVoBvAKGmgHoAqVxp16AAQAcwAJzAU6AewBGAKwTAUAX2hi+KCJ2jd+QkXQgpBAS3QBzRQG1lqgHTAAroMEZgATwC661s07AwIEAQQAClo7BydneQBKOnVoRxR7dGhQx3QXaABqaAByAEYABit8-OyAbk55enkgA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants