Skip to content

False positive ownership_invalid_binding? #15210

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.

Sign up for GitHub

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

Jump to bottom
Closed
donghokimm opened this issue Feb 5, 2025 · 4 comments · Fixed by #15678
Closed

False positive ownership_invalid_binding? #15210

donghokimm opened this issue Feb 5, 2025 · 4 comments · Fixed by #15678
Labels
bug

Comments

@donghokimm
Copy link

Describe the bug

I've encountered the ownership_invalid_binding warning while working on a Svelte 5 project.

Reading the documentation was not helpful in fixing this issue.

Can you help me?

Reproduction

REPL

Logs



System Info


System:
  OS: Windows 11 10.0.26100
  CPU: (18) x64 Intel(R) Core(TM) Ultra 5 125H
  Memory: 4.40 GB / 15.74 GB
Binaries:
  Node: 22.11.0 - C:\Program Files\nodejs\node.EXE
  npm: 10.9.0 - ~\AppData\Roaming\npm\npm.CMD
Browsers:
  Edge: Chromium (131.0.2903.99), ChromiumDev (133.0.3014.0)
  Internet Explorer: 11.0.26100.1882


Severity


annoyance

      		
    

      
  





        



            
            

              
            

        

      


      
    








      

    



      

  
            

    

      
    

    


      

          @dummdidumm
dummdidumm




          added
  the 

  bug

 label


      Feb 5, 2025

    

  








      

  
      



  

  @dummdidumm





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      

  
  Member


      

  


  

    

  





      


        


  
    

      
          
The problem is that ownership is not widened correctly - it does not know about the previous owners because it's a class which is not traversed. For proxies we have logic in place for this, for classes we need something similar.

      		
    

  





        


            

            
            

              
            

        

      


      

            
  

    
  
    
    

  

  


    












      

  
      



  

  @mcullifer





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  





      


        


  
    

      
          
@dummdidumm So what would be the recommendation here? If we know the warning is due to it being class just ignore? Or do you recommend not using classes at all? I think I'm encountering a similar issue to this where I'm using a context which has a class as value. When child components get that context and mutate a property on the class I get the warning.

      		
    

  





        


            

            
            

              
            

        

      


      

            
  

    
  
    
    

  

  


    












      

  
      



  

  @dm-de





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

      

      
            dm-de
  

      

      

      commented


        Feb 5, 2025


      
    


  





      


        


  
    

      
          
This warning is confusing and useless.

I found out that it's all about where the value is changed. With snippets you can "bend" that - and the change takes place in other components.


see here:

#14145 (comment)

      		
    

  





        


            

            
            

              
            

        

      


      

            
  

    
  
    
    

  

  


    












      

  
      



  

  @donghokimm





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  Author


  


  

    

  





      


        


  
    

      
          
@dummdidumm

I appreciate the explanation!

Is there a plan to fix it?

      		
    

  





        


            

            
            

              
            

        

      


      

            
  

    
  
    
    

  

  


    












      

  
          


  
    
  
  

    
  dummdidumm 

      added a commit
      that referenced
      this issue

    
      Apr 3, 2025
    
    

      


  

    

      


  

      
        @dummdidumm
  





      

        
          fix: rework binding ownership validation
        

          
            
          

      


      

          

    
    
    

  

      


      

      


      
      

        
          88a83d6
        
      

    

  

    

      
Previously we were doing stack-based retrieval of the owner, which while catching more cases was costly (performance-wise) and prone to errors (as shown by many issues over the months).

This drastically simplifies the ownership validation - we now only do simple static analysis to check which props are mutated and wrap them with runtime checks to see if a binding was established.

Besides making the implementation simpler and more performant, this also follows an insight we had over the months: Most people don't really know what to do with this warning when it's shown beyond very simple cases. Either it's not actionable because they don't really know how to fix it or they question if they should at all (in some cases rightfully so). Now that the warning is only shown in simple and easy-to-reason-about cases, it has a much better signal-to-noise-ratio and will hopefully guide people in the right direction early on (learn from the obvious cases to not write spaghetti code in more complex cases).

closes #15532
closes #15210
closes #14893
closes #13607
closes #13139
closes #11861

    




    

  




  

  

    
  


  

      

    @dummdidumm
dummdidumm



    mentioned this issue
    
      Apr 3, 2025
    





  


  

      
   Merged

  





  
  

      
        
      
      

  
    
    
  


    6 tasks
  





  




    

    


    

    


  









  
  

    

      

  





  




  
  

              

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  



  






  
                  




    

  


      
  

    Assignees
  



        

    No one assigned







      


  


  

    Labels
  



    

      

    bug









      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      No milestone





    
      
          


  

    

      
        

          
    

    Development
  




            
  
Successfully merging a pull request may close this issue.





    


  
          
      fix: rework binding ownership validation

    
      sveltejs/svelte
    





      
    

  




      

    

  
      

  

    

      4 participants
    

    

        
          @dummdidumm 
        
          @mcullifer 
        
          @dm-de 
        
          @donghokimm