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

feat: export initialized socket client #4304

Merged
merged 4 commits into from Apr 5, 2022

Conversation

pmmmwh
Copy link
Contributor

@pmmmwh pmmmwh commented Feb 20, 2022

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

No new tests are added as I believe this is a simple change and no additional functionality is added, it just exposes a bit more of the private parts.

Added a simple test for null value before init, and a "valid" client instance after init.

Motivation / Use-Case

Fixes #4261
Fixes #4324

Additional Info

I chose the simplest way to expose the initialized client, so consumers can just import the file and use it - but I'm also ok with setting it to some globals (on globalThis/window) if you think that's better (I'm unsure if mutable exports are implemented properly when compiled?).

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 20, 2022

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #4304 (13f2d5b) into master (4e7800e) will not change coverage.
The diff coverage is n/a.

❗ Current head 13f2d5b differs from pull request most recent head b91ae7d. Consider uploading reports for the commit b91ae7d to get more accurate results

@@           Coverage Diff           @@
##           master    #4304   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files          15       15           
  Lines        1549     1549           
  Branches      591      591           
=======================================
  Hits         1428     1428           
  Misses        112      112           
  Partials        9        9           

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 4e7800e...b91ae7d. Read the comment docs.

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

Let's fix lint.

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Feb 21, 2022

I'm not sure if I should just do eslint ignore here, or should I do something more

@alexander-akait
Copy link
Member

Just add eslint-plugin-ignore and small comment(s) why we need to it

@alexander-akait
Copy link
Member

@pmmmwh Can we add small test cases (where we try to get client) to avoid future regressions?

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Apr 4, 2022

Added test!

@pmmmwh pmmmwh force-pushed the patch-1 branch 3 times, most recently from 29ad8cd to a7a2d38 Compare April 4, 2022 17:40
@pmmmwh
Copy link
Contributor Author

pmmmwh commented Apr 4, 2022

Completely rebased onto latest HEAD + fixed commitlint issue 😄

@alexander-akait alexander-akait merged commit 7920364 into webpack:master Apr 5, 2022
@alexander-akait
Copy link
Member

Thanks

@alexander-akait
Copy link
Member

@pmmmwh pmmmwh deleted the patch-1 branch April 5, 2022 19:15
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.

Expose Socket Client for external integrations
3 participants