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(signal-slice): add lazySources #197

Merged
merged 8 commits into from
Dec 10, 2023

Conversation

joshuamorony
Copy link
Contributor

@joshuamorony joshuamorony commented Dec 9, 2023

EDIT: The note below has been resolved

NOTE: This is a draft PR due to one remaining issue and I'm not sure if it can be solved with this set up. I think the crux of the issue is that I am relying on a proxy to detect when the state object is accessed in order to trigger the lazySources to connect:

  return new Proxy(slice, {
    get(target, property, receiver) {
      if (!lazySourcesLoaded) {
        lazySourcesLoaded = true;
        connectSources(state, lazySources, injector);
      }

      return Reflect.get(target, property, receiver);
    },
  });

Although it still seems to work (the observable values are still set into the signal), it triggers the SIGNAL_WRITE_FROM_ILLEGAL_CONTEXT error:

ERROR Error: NG0600: Writing to signals is not allowed in a `computed` or an `effect` by default. Use `allowSignalWrites` in the `CreateEffectOptions` to enable this inside effects.

This adds support for lazySources as discussed here: #196

It allows specifying sources and lazySources in the config - both are identical but the lazySources will not be supplied to the connect function until the state object has been accessed.

I have left this undocumented for now, ideally would like to have it used a little first. I think I also need to do a full sweep through and improve the docs as a whole as well.

Copy link

nx-cloud bot commented Dec 9, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0965803. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


⌛ The following target is in progress

✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

@nartc
Copy link
Collaborator

nartc commented Dec 9, 2023

I can help with the issue

@nartc
Copy link
Collaborator

nartc commented Dec 9, 2023

Check this commit for fix to issue

@nartc nartc force-pushed the feature/signal-slice-lazy-sources branch from 2d79806 to 0965803 Compare December 9, 2023 17:07
@joshuamorony joshuamorony marked this pull request as ready for review December 9, 2023 20:48
@joshuamorony
Copy link
Contributor Author

Perfect, this seems to work, thank you! This should be ready to go now

@nartc nartc merged commit 44cf5f9 into ngxtension:main Dec 10, 2023
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