jackbsteinberg / get-originals-rewriter

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Tests to write/cleanup #15

Closed domenic closed 5 years ago

domenic commented 5 years ago

After #14 lands:

jackbsteinberg commented 5 years ago

These testing fixes will come in the next PR

jackbsteinberg commented 5 years ago

Update: ts-morph is having a lot of trouble figuring out the case where a constructor is passed to a function, so the third bullet point will be left as polish if we decide it becomes necessary.

domenic commented 5 years ago

Update: ts-morph is having a lot of trouble figuring out the case where a constructor is passed to a function, so the third bullet point will be left as polish if we decide it becomes necessary.

I did a bit more digging. The issue is basically there's no reasonable type for the constructors. E.g. in the snippet

const y = Document;

what is the type of y? https://ts-ast-viewer.com/ indicates that it is (something that stringifies as) { new (): Document; prototype: Document; }.

And indeed if I put as the input file

/**
 * @param {{ new (): Document }} api - the api to call
 */
function f(api) {
  const a = new api();
  return a;
}

const output = f(Document);

then TypeScript will figure it out. So I guess we could add such a test...

jackbsteinberg commented 5 years ago

Is that something we want to test for? Or does it make more sense to decide not to pass Global constructors into methods which:

  1. Doesn't make much sense anyway, if they're global then by definition you can access them from anywhere.

  2. Is probably a style we already follow, just has yet to be formalized.

domenic commented 5 years ago

Yeah, I'd be fine declaring it out of scope. It's just that I think it'll work already anyway with the technique we're using, so adding a test might be useful to ensure we don't break it. I'm OK either way.