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 IJSObjectReference support / JS Module support #288

Merged
merged 5 commits into from
Dec 19, 2020

Conversation

egil
Copy link
Member

@egil egil commented Dec 14, 2020

Pull request description

PR meta checklist

  • Pull request is targeting at DEV branch.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Content checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@egil egil linked an issue Dec 14, 2020 that may be closed by this pull request
@egil
Copy link
Member Author

egil commented Dec 14, 2020

Hey @KristofferStrube, this is my initial draft. There are still tests missing, and at least some clean up of the code needed. But let me know what you think about the API. This is probably most easily done through tests/bunit.web.tests/JSInterop/BunitJSModuleInteropTest.cs.

@codecov
Copy link

codecov bot commented Dec 14, 2020

Codecov Report

Merging #288 (a69fed1) into dev (5c668cb) will increase coverage by 0.13%.
The diff coverage is 90.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #288      +/-   ##
==========================================
+ Coverage   82.64%   82.77%   +0.13%     
==========================================
  Files         118      126       +8     
  Lines        3612     3710      +98     
  Branches      468      481      +13     
==========================================
+ Hits         2985     3071      +86     
- Misses        480      491      +11     
- Partials      147      148       +1     
Impacted Files Coverage Δ
src/bunit.core/ComponentParameterCollection.cs 82.14% <ø> (ø)
src/bunit.core/RazorTesting/FixtureBase.cs 94.44% <ø> (ø)
src/bunit.core/RazorTesting/FragmentContainer.cs 100.00% <ø> (ø)
...b/Asserting/FocusAsyncAssertJSInteropExtensions.cs 100.00% <ø> (ø)
...it.web/Asserting/JSInvokeCountExpectedException.cs 54.83% <ø> (ø)
...it.web/Extensions/TestServiceProviderExtensions.cs 100.00% <ø> (ø)
...JSInterop/JSRuntimeUnhandledInvocationException.cs 100.00% <ø> (ø)
src/bunit.web/RazorTesting/Fixture.cs 95.65% <ø> (+1.44%) ⬆️
src/bunit.web/RazorTesting/SnapshotTest.cs 78.57% <ø> (ø)
src/bunit.web/Rendering/Internal/Htmlizer.cs 83.63% <ø> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c668cb...a69fed1. Read the comment docs.

@KristofferStrube
Copy link
Contributor

I've looked through the tests and it looks good.

I tried to modify one of the tests to test a custom import without any parameters. Then we have to write the following:

JSInterop.SetupModule("customImport", Array.Empty<object>());
var module = await JSInterop.JSRuntime.InvokeAsync<IJSObjectReference>("customImport");

to specify that this is a custom import function with no arguments.

I'm not sure if that is intuitive but it would of course cause an overload of the SetupModule(moduleName) method if the arguments were to become optional, so I think it was the correct solution you made.

Overall looking good. 👍

@egil
Copy link
Member Author

egil commented Dec 15, 2020

That was indeed my point about making the args array required. I did consider having a SetupCustomModule, but it felt a bit like too much work for a use case that is not likely to be used a lot, i think.

If folks need that in their lives, they can easily add it though, it's just an extension methods.

@egil egil marked this pull request as ready for review December 19, 2020 13:35
@egil egil merged commit 0b7692c into dev Dec 19, 2020
@egil egil deleted the feature/233-IJSObjectReference-Support branch December 19, 2020 13:36
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.

Add support for JSObjectReference/IJSObjectReference
2 participants