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

Make consume and ContextConsumer return undefined rather than null #31

Merged

Conversation

boris-petrov
Copy link
Contributor

Fixes #29

cc @kevinkucharczyk

I'm not sure I've done the change correctly - running npm lint locally seems to lead to a lot of TypeScript errors which I'm unsure how to fix. Please take a look and let me know if I've done something wrong.

Copy link

changeset-bot bot commented Sep 9, 2024

🦋 Changeset detected

Latest commit: a894999

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kevinkucharczyk
Copy link
Collaborator

@boris-petrov what TypeScript errors are you seeing? When I run it locally I see a few warnings:
CleanShot 2024-09-10 at 08 44 42@2x

But they were before - nothing related to your changes. There are no errors and the linting passes correctly.

@boris-petrov
Copy link
Contributor Author

Well, running npm run lint leads to these:

[lint:types] app/components/a-component.ts(2,25): error TS2307: Cannot find module 'ember-provide-consume-context' or its corresponding type declarations.
[lint:types] app/components/custom-provider.ts(2,25): error TS2307: Cannot find module 'ember-provide-consume-context'or its corresponding type declarations.
[lint:types] app/components/custom-provider.ts(3,34): error TS2307: Cannot find module 'ember-provide-consume-context/context-registry' or its corresponding type declarations.
[lint:types] app/components/custom-provider.ts(28,16): error TS2664: Invalid module name in augmentation, module 'ember-provide-consume-context/context-registry' cannot be found.
[lint:types] app/components/test-component.ts(2,25): error TS2307: Cannot find module 'ember-provide-consume-context' or its corresponding type declarations.
[lint:types] app/components/test-component.ts(3,34): error TS2307: Cannot find module 'ember-provide-consume-context/context-registry' or its corresponding type declarations.
[lint:types] app/controllers/application.ts(33,16): error TS2664: Invalid module name in augmentation, module 'ember-provide-consume-context/context-registry' cannot be found.
[lint:types] tests/integration/components/decorators-test.ts(7,34): error TS2307: Cannot find module 'ember-provide-consume-context' or its corresponding type declarations.
[lint:types] tests/integration/components/test-support-test.ts(8,8): error TS2307: Cannot find module 'ember-provide-consume-context/test-support' or its corresponding type declarations.
[lint:types] types/global.d.ts(3,47): error TS2307: Cannot find module 'ember-provide-consume-context/template-registry' or its corresponding type declarations.

And below there's a lot more. One of them:

[lint:types] app/templates/application.hbs:75:13 - error TS7053: Unknown name 'ContextProvider'. If this isn't a typo,you may be missing a registry entry for this value; see the Template Registry page in the Glint documentation for moredetails.
[lint:types]   Element implicitly has an 'any' type because expression of type '"ContextProvider"' can't be used to index type 'Globals'.
[lint:types]     Property 'ContextProvider' does not exist on type 'Globals'.
[lint:types]
[lint:types]  75             <ContextProvider @key="NumberContext" @value={{9}}>
[lint:types]                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[lint:types]  76               <BComponent />
[lint:types]     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[lint:types] ...
[lint:types]  79               </ContextConsumer>
[lint:types]     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[lint:types]  80             </ContextProvider>
[lint:types]     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That doesn't happen when you clone the repo, npm i and then npm run lint?

@kevinkucharczyk
Copy link
Collaborator

@boris-petrov I am not seeing those on my machine! What versions of node and npm are you on?

@boris-petrov
Copy link
Contributor Author

Node v22.8.0, npm 10.8.3. I don't think these should matter in that case though.

@kevinkucharczyk
Copy link
Collaborator

@boris-petrov I can't seem to reproduce what you're seeing. You said you weren't sure if your changes are correct - does this mean running npm lint against the main branch works fine for you?

@boris-petrov
Copy link
Contributor Author

No, the same thing happens on main, that's why I can't be sure that my changes are correct. It's really strange that you don't see what I see. As I said, my setup is pretty ordinary - Arch Linux, latest Node/npm, but these things shouldn't really matter. But in any case - CI here is happy so the problem should be somewhere on my machine. Not sure if we can also add/change a test somewhere for this PR?

@kevinkucharczyk
Copy link
Collaborator

@boris-petrov

Not sure if we can also add/change a test somewhere for this PR?

We can definitely add some tests!
There are integration test under test-app:

It would make sense to add a test case in each to test what the result is when no provider is present.

Would you like to add those?

@boris-petrov
Copy link
Contributor Author

boris-petrov commented Oct 5, 2024 via email

@boris-petrov
Copy link
Contributor Author

@kevinkucharczyk I've added two tests for the two changes. I also found why I was getting these errors. Apparently one must build an addon before one can use it. 😄 Didn't know that. So once I ran npm run build I could get both linting and tests running fine.

@kevinkucharczyk kevinkucharczyk merged commit 446d9ca into customerio:main Oct 14, 2024
12 checks passed
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.

Types for ContextConsumer
2 participants