ember-resources icon indicating copy to clipboard operation
ember-resources copied to clipboard

Create eslint plugin

Open NullVoxPopuli opened this issue 3 years ago • 5 comments

Goal is to help folks out with issues like what is reported here: https://github.com/NullVoxPopuli/ember-resources/issues/707

Todo: Lints for

  • [ ] warn: short circuiting logic can prevent other data from ever being consumed

    if (foo && bar) { /* ... */ }
    

    in this situation, if both foo and bar are tracked, and foo is always false, the resource will never add bar to it's list of reactive dependencies

  • [ ] against: setting variables on this within a resource

    • [ ] docs and explanation
    • [ ] within trackedFunction
      class Demo {
        // ❌ bad
        /* ... */ = trackedFunction(() => {
          this.prop = 1;
          /* ... */
        });
      }
      
    • [ ] within resource()
      class Demo {
        // ❌ bad
        /* ... */ = resource(() => {
          this.prop = 1;
          /* ... */
        });
      }
      
    • [ ] within argument thunk (args to a resource blueprint/factory/from)
      class Demo {
        // ❌ bad
        /* ... */ = MyResource.from(() => {
          this.prop = 1;
          return [ ... ];
        });
        // ❌ bad
        /* ... */ = RemoteData(() => {
          this.prop = 1;
          return 'https://...';
        });
      }
      
  • [ ] against: setting tracked state after the state was created (unless inside an awaited IIFE)

    class Demo {
      // ❌ bad
      /* ... */ = resource(() => {
        let state = new TrackedObject();
        state.data = 2
        /* ... */
      });
    
      // ✅ good
      /* ... */ = resource(() => {
        let state = new TrackedObject({ data: 2 });
    
        /* ... */
      });
    
      // ✅ good
      /* ... */ = resource(() => {
        let state = new TrackedObject();
    
        (async () => {
          state.data = 2        
        })();
        /* ... */
      });
    
      }
    
  • [ ] autofix: if this is accessed after an await, suggested and fix with destroyable protection (if isDestroyed || isDestroying) -- see also: https://github.com/ember-cli/eslint-plugin-ember/pull/1421

 class Demo {
   // ❌ bad
   /* ... */ = resource(() => {
     (async () => {
       await Promise.resolve();
       let foo = this.foo;
     })();
     /* ... */
   });

   // ✅ good
   /* ... */ = resource(() => {
     (async () => {
       await Promise.resolve();
       if (isDestroying(this) || isDestroyed(this)) return;

       let foo = this.foo;
     })();        
     /* ... */
   });

Stretch goal lints

  • [ ] against: if a function call within a resource is within the same file, and if it sets properties on this
  • [ ] strict against: all resources defined outside of a component (to force encapsulation)

NullVoxPopuli avatar Dec 17 '22 17:12 NullVoxPopuli

🦋 Changeset detected

Latest commit: 566d0e2ea5ea6dc262e0f26bebf468964d676b7c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-ember-resources Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Dec 17 '22 17:12 changeset-bot[bot]

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 427c4a1
Status: ✅  Deploy successful!
Preview URL: https://8ebed39f.ember-resources.pages.dev
Branch Preview URL: https://create-eslint-plugin.ember-resources.pages.dev

View logs

Estimated impact to a consuming app, depending on which bundle is imported

js min min + gzip min + brotli
/index.js 13.97 kB 3.33 kB 1.36 kB 1.2 kB
├── core/class-based/index.js 4.43 kB 1.88 kB 929 B 798 B
├── core/function-based/index.js 6.03 kB 552 B 269 B 213 B
└── core/use.js 2.91 kB 415 B 256 B 203 B
/util/cell.js 2.28 kB 917 B 434 B 366 B
/util/debounce.js 2.64 kB 771 B 409 B 340 B
/util/ember-concurrency.js 4.33 kB 1.53 kB 733 B 626 B
/util/function-resource.js 216 B 154 B 123 B 87 B
/util/function.js 4.82 kB 2.99 kB 1.05 kB 905 B
/util/helper.js 1.79 kB 303 B 218 B 177 B
/util/keep-latest.js 1.75 kB 512 B 296 B 235 B
/util/map.js 5.19 kB 2.13 kB 918 B 794 B
/util/remote-data.js 4.86 kB 1.75 kB 675 B 596 B

github-actions[bot] avatar Dec 17 '22 17:12 github-actions[bot]

@NullVoxPopuli, wondering if we could land MVP version of this plugins and iterate on it.

lifeart avatar Jan 23 '25 07:01 lifeart

short circuiting logic can prevent other data from ever being consumed if (foo && bar) { /* ... */ } in this situation, if both foo and bar are tracked, and foo is always false, the resource will never add bar to it's list of reactive dependencies

If I understand this correctly it is the same for getters and if so I disagree with this rule. I've thought about it a lot and the thing is the reactivity to bar is a no-op anyway if foo is falsey. Thus it doesn't make much difference. As foo would have to react first before bar even came into the concerns of being consumed.

Basically, the notion that bar needs to be consumed (registered as reactive) only matters when foo becomes truthy. For example, foo = false, bar = we don't care, In this case we wait till foo = true causing the code to re-execute under another consumption context in which case the second round foo is truthy and bar becomes consumed then registering it as reactive.

The logic works out and the worry that bar has any effect on the re-calculation of the resource when foo is falsey is a mental model mismatch. I don't see how an eslint rule prevents any foot guns in this case.

sukima avatar Jan 23 '25 14:01 sukima