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

Resolve issue with first displayName always being used #449

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
name: Node.js Package

on:
push:
branches:
- '**'
release:
types: [created]

Expand Down
22 changes: 22 additions & 0 deletions src/__tests__/data/MultipleAllWithDisplayName.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import * as React from 'react';

interface ButtonProps
extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'type'> {}

export const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
(props, ref) => <button {...props} ref={ref} type="button" />
);

Button.displayName = 'First';

export const SubmitButton = React.forwardRef<HTMLButtonElement, ButtonProps>(
(props, ref) => <button {...props} ref={ref} type="submit" />
);

SubmitButton.displayName = 'Second';

export const ResetButton = React.forwardRef<HTMLButtonElement, ButtonProps>(
(props, ref) => <button {...props} ref={ref} type="reset" />
);

ResetButton.displayName = 'Third';
18 changes: 18 additions & 0 deletions src/__tests__/data/MultipleSomeWithDisplayName.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as React from 'react';

interface ButtonProps
extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'type'> {}

export const Button1 = React.forwardRef<HTMLButtonElement, ButtonProps>(
(props, ref) => <button {...props} ref={ref} type="button" />
);
Button1.displayName = 'First';

export const NoExplicitDisplayName1 = (props: any) => <div>Some text</div>;

export const Button2 = (props: ButtonProps) => (
<button {...props} type="button" />
);
Button2.displayName = 'Second';

export const NoExplicitDisplayName2 = (props: any) => <div>Some more text</div>;
14 changes: 14 additions & 0 deletions src/__tests__/data/MultipleWithNoExplicitDisplayName.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import * as React from 'react';

interface ButtonProps
extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'type'> {}

export const Button = (props: ButtonProps) => (
<button {...props} type="button" />
);
export const SubmitButton = (props: ButtonProps) => (
<button {...props} type="submit" />
);
export const ResetButton = (props: ButtonProps) => (
<button {...props} type="reset" />
);
42 changes: 42 additions & 0 deletions src/__tests__/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,48 @@ describe('parser', () => {
const [parsed] = parse(fixturePath('StatefulDisplayNameFolder/index'));
assert.equal(parsed.displayName, 'StatefulDisplayNameFolder');
});

describe('multiple components in one', () => {
it('should parse all `displayName` properties correctly when all are explicitly defined.', () => {
const result = parse(fixturePath('MultipleAllWithDisplayName'));

// Ensure we're not missing any exports.
assert.equal(result.length, 3);

const [parsed1, parsed2, parsed3] = result;

assert.equal(parsed1.displayName, 'First');
assert.equal(parsed2.displayName, 'Second');
assert.equal(parsed3.displayName, 'Third');
});

it('should parse all `displayName` properties correctly when some are explicitly defined.', () => {
const result = parse(fixturePath('MultipleSomeWithDisplayName'));

// Ensure we're not missing any exports.
assert.equal(result.length, 4);

const [parsed1, parsed2, parsed3, parsed4] = result;

assert.equal(parsed1.displayName, 'First');
assert.equal(parsed2.displayName, 'NoExplicitDisplayName1');
assert.equal(parsed3.displayName, 'Second');
assert.equal(parsed4.displayName, 'NoExplicitDisplayName2');
});

it('should parse all `displayName` properties correctly when none are explicitly defined.', () => {
const result = parse(fixturePath('MultipleWithNoExplicitDisplayName'));

// Ensure we're not missing any exports.
assert.equal(result.length, 3);

const [parsed1, parsed2, parsed3] = result;

assert.equal(parsed1.displayName, 'Button');
assert.equal(parsed2.displayName, 'SubmitButton');
assert.equal(parsed3.displayName, 'ResetButton');
});
});
});

describe('Parser options', () => {
Expand Down
33 changes: 32 additions & 1 deletion src/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1211,11 +1211,42 @@ function getTextValueOfFunctionProperty(
.filter(statement => {
const expr = (statement as ts.ExpressionStatement)
.expression as ts.BinaryExpression;

const locals = Array.from(
(source as any).locals as [string, ts.Symbol][]
);
const hasOneLocalExport =
locals.filter(local => !!local[1].exports).length === 1;
Comment on lines +1215 to +1219
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't certain about this, but it did seem to resolve the one failing test I was dealing with.

Specifically: 'should be taken from stateless component displayName property (using default export) even if file name doesn't match'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvasek @jesstelford curious on any input here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produced false-negatives in my scenario. I presume because there is only one with displayName, it was ignoring the others.

Copy link
Contributor Author

@kylemh kylemh Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AMoo-Miki If you log exports there, what's the output?


if (hasOneLocalExport) {
return (
expr.left &&
(expr.left as ts.PropertyAccessExpression).name &&
(expr.left as ts.PropertyAccessExpression).name.escapedText ===
propertyName
);
}

/**
* Ensure the .displayName is for the currently processing function.
*
* This avoids the following situations:
*
* - A file has multiple functions, one has `.displayName`, and all
* functions ends up with that same `.displayName` value.
*
* - A file has multiple functions, each with a different
* `.displayName`, but the first is applied to all of them.
*/
const flowNodeNameEscapedText = (statement as any)?.flowNode?.node?.name
?.escapedText as false | ts.__String | undefined;

return (
expr.left &&
(expr.left as ts.PropertyAccessExpression).name &&
(expr.left as ts.PropertyAccessExpression).name.escapedText ===
propertyName
propertyName &&
flowNodeNameEscapedText === exp.escapedName
);
})
.filter(statement => {
Expand Down
Loading