- 
                Notifications
    
You must be signed in to change notification settings  - Fork 29
 
Build the proposal API on top of alien-signals #44
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
base: main
Are you sure you want to change the base?
Conversation
| 
           @NullVoxPopuli Did you add prohibited-contexts.test? Does this test need to be updated or is there a genuine issue here do you think?  | 
    
| 
           I did not add it -- it was implemented with the original spec -- but I did pull it out of a massive test file, as we need to be describing why tests exist, and what they're testing, and why that behavior is important. It is def a behavior question about whether or not folks want to allow synchronous mutation while reading another value. Personally, I don't think this is a good idea, as it prior-reads of the mutated state are now out of date, and if the consumer is entangled with the mutated state, that usually leads to infinite looping when a renderer is involved  | 
    
| 
           We have already solved the problem of infinite loops at the algorithmic level. If synchronous mutations in computed is not handled, it can not pass the Vue core test suite.  | 
    
| 
           Sounds fine since it's solved.  | 
    
| 
           Can you link to the source of the benchmark? I'd be curious to see what the test setup was like. Was it based on https://github.com/transitive-bullshit/js-reactivity-benchmark?  | 
    
0ffa34f    to
    e6f28d6      
    Compare
  
    | 
           @jkrems yes, I just add a test for this PR to a new branch. https://github.com/johnsoncodehk/js-reactivity-benchmark/tree/alien-polyfill  | 
    
| 
           @johnsoncodehk I was asking because that benchmark has some known measuring artifacts for the signal polyfill specifically (see https://x.com/synalx/status/1868235387812053167). So it might be less predictive for this particular PR. That doesn't mean that this PR isn't a performance improvement. But it might just require additional validation before it's clear how it compares.  | 
    
| 
           Thank you so much for this work @johnsoncodehk! I believe @EisenbergEffect said offline he was also in favor of this general approach? Does anyone have concerns about moving in this direction? I think maybe I had heard conversations where some application use cases found other approaches to be faster? It's a large change so I'm hesitant for us to merge it in without a bit more conversation / consensus but would love to hear how people are feeling!  | 
    
| // would cause it to change value (due to the set inside of it). | ||
| expect(c.get()).toBe(2); | ||
| expect(s.get()).toBe(2); | ||
| if (isAlien) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when are we not alien?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always alien, this IF branch is just to demonstrate the difference between the old and new behavior.
        
          
                src/alien.ts
              
                Outdated
          
        
      | @@ -0,0 +1,360 @@ | |||
| import * as alien from 'alien-signals'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we get any perf benefits by internalizing the implementation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how much internalizing can simplify the alien's createReactiveSystem abstraction. Based on my experience internalizing alien into Vue, the potential performance improvement is likely negligible (<5%).
| 
           Hi, any updates? Can't wait to see the new stuff! :D 
 Unifying reactivity systems at this level would be fantastic! Big thanks for your awesome work! 🤗  | 
    
      
        
              This comment was marked as resolved.
        
        
      
    
  This comment was marked as resolved.
7e2ad4d    to
    e61395d      
    Compare
  
    | 
           I think the PR is complete! The implementation has been simplified a lot while maintaining as much consistency as possible with the original behavior. To achieve this, surface APIs have added checks for isState, isComputed, and isWatcher, which will affect performance. The initial performance comparison may no longer be applicable, but the improvements should still be significant.  | 
    
We are re-constructing surface APIs based on alien-signals to obtain performance improvements, now faster than most frameworks.
We intentionally relies on the alien-signals package rather than duplicating code in order to easily discover code specific to the signal proposal.
Regarding the differences in test results.
Prohibited contexts - allows writes during computed: The alien-signals algorithm is able to handle computed side effects, so the expected results in the test are now modified to the correct values.(resolved)type checks - checks types in methods: I'm not sure what I should do to make the current implementation pass these tests, but since this PR is for research purposes only, I don't think this test is worth solving, so I just skipped it.Chart: