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

docs: change return value of page-title helper #330

Closed
wants to merge 1 commit into from

Conversation

muziejus
Copy link

The code for the page-title helper includes:

  compute(params, _hash) {
    let hash = {
      ..._hash,
      id: this.tokenId,
      title: params.join(''),
    };

    this.tokens.push(hash);
    this.tokens.scheduleTitleUpdate();
    return '';
  }

https://github.com/ember-cli/ember-page-title/blob/9f3a3b979a8bbca9e41f5c274993a964dc0fa461/addon/src/helpers/page-title.js#L24

The return value of the helper is an empty string, and not void, so
I've updated the signature typing in the glint documentation.

The code for the `page-title` helper includes:

```js
  compute(params, _hash) {
    let hash = {
      ..._hash,
      id: this.tokenId,
      title: params.join(''),
    };

    this.tokens.push(hash);
    this.tokens.scheduleTitleUpdate();
    return '';
  }
```

https://github.com/ember-cli/ember-page-title/blob/9f3a3b979a8bbca9e41f5c274993a964dc0fa461/addon/src/helpers/page-title.js#L24

The return value of the helper is an empty string, and not `void`, so
I've updated the signature typing in the glint documentation.
@dfreeman
Copy link
Member

Thank you for the PR—it's always wonderful to see improvements to the docs here! In this instance, however, I don't think we want to make this change.

While it's true that the implementation of that helper returns '', from a practical perspective what it's modeling is closer to an effect. The page-title docs only ever direct users to invoke the helper as a top-level item in the template, not that its return value is something that should be consumed.

If a user were to write something like this:

<MyComponent @aStringTypeArg={{page-title "hello"}} />

That would almost certainly be a misuse of the helper and indicate a misunderstanding on the user's part of how it works, but Return: '' would allow that to typecheck. Note that void as a return type does not necessarily mean "returns undefined", but rather "returns something whose type won't be observed".

@muziejus
Copy link
Author

Thanks, @dfreeman. I understand conceptually what you mean, and the stack overflow answer was illuminating, but I still get hung up over the fact that if I do:

const foo = ():void => ("")

I get an error from the TS checker. Yet from how I understand the Stack Overflow response, this isn't an "error" but rather a warning, or something:

TypeScript will stop you from "accidently" returning a value, even though this wouldn't create a type system violation. This is helpful for catching bugs that appear from a refactoring

The real solution, as indicated here, seems to be just binning the return value :) So I'll aim my efforts there.

@muziejus muziejus closed this May 29, 2022
@dfreeman
Copy link
Member

TypeScript has a number of "what you've done is typesafe but probably not what you meant, so I'm flagging it" errors. The most common one is its excess property checking, which triggers when you use an object literal directly in a typed position but not when you use a variable of the same type:

declare function greet(options: { message: string }): void;

let options = { message: 'Hello', scope: 'World' };
greet(options); // ✅ typechecks

greet({ message: 'Hello', scope: 'World' }); // ❌ error TS2345

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