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

refactor: add operators with addOperators in context constructor #391

Merged
merged 1 commit into from
Oct 19, 2023
Merged

refactor: add operators with addOperators in context constructor #391

merged 1 commit into from
Oct 19, 2023

Conversation

maxnowack
Copy link
Contributor

@maxnowack maxnowack commented Oct 19, 2023

Spreading the imports to objects converts them to plain objects again and resolves the issues discussed in #390
resolves #390

Update: see this comment

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #391 (c90fcaa) into main (d17d517) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
- Coverage   98.75%   98.73%   -0.03%     
==========================================
  Files         290      290              
  Lines        4193     4195       +2     
  Branches      985      985              
==========================================
+ Hits         4141     4142       +1     
- Misses         46       47       +1     
  Partials        6        6              
Flag Coverage Δ
unittests 98.73% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/core.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kofrasa
Copy link
Owner

kofrasa commented Oct 19, 2023

Thanks for the PR @maxnowack. If copying the input resolves it, perhaps it would be better done within the operator register methods themselves to prevent other users from running into the same issue.

Consider updating useOperator and Context.addOperator functions instead if doing so would have the same effect.

Thoughts?

@maxnowack
Copy link
Contributor Author

@kofrasa thanks for pointing me in this direction. I tried to verify this and realized that my reproduction environment wasn't up to date with the latest changes and that my proposed changes don't make sense at all.
The Context.addOperators (which is called by useOperators) method is already implicitly copying the object by looping over Object.entries.

My reproduction environment was using v6.4.7 instead of the latest changes on main. As I wasn't able to reproduce the TypeError with the latest changes, I rechecked the callstack of the error and saw the the call were not coming from useOperators but from Context.init.
The behavior how operators were loaded has changed with 254afa6 (here). As the context is now initialized with an empty object and the operators will be added explicitly with the corresponding methods, the TypeError is already fixed on main.

However, I just changed this PR to modify the Context.constructor to also use addOperators method internally. This ensures the copying of the operators regardless if the context were initialized with the operators or the operators were added after initialization.

@maxnowack maxnowack changed the title fix: spread imports into plain object to resolve build env issues refactor: add operators with addOperators in context constructor Oct 19, 2023
@kofrasa kofrasa merged commit 054bcb1 into kofrasa:main Oct 19, 2023
7 of 8 checks passed
@maxnowack maxnowack deleted the fix-typeerror branch October 19, 2023 22:27
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.

TypeError: Cannot convert undefined or null to object
2 participants