Type-check blueprint files

[simonihmig]

First step in an attempt to fix #4.

For the beginning this adds a (failing) test to type-check the blueprint output. Actually fixing the output will follow over time... (at least waiting for #74 to be merged)

The test makes sure that files generated by the blueprints type-check correctly. As all tests use fixture files to test that the blueprints generate the correct output, we can just add a linting test that type-checks the existing static fixtures files, instead of running the blueprints and type-checking the output afterwards.

To make imports not cause type errors, the appropriate typing packages have been added to devDependencies, and types/index.d.ts declares all in-app modules used in the test fixtures.

Anyone here have any concerns with the approach taken here so far? @chriskrycho @mike-north

[chriskrycho]

This reply is five months late in coming, mostly because I have not made the time for thinking through the blueprints space. I apologize!

Per the various discussions we’ve had around #74 and #71, I’m unsure how we should proceed. I’d like to talk through this point with some Ember CLI folks to see if we can design a better solution. In the meantime, I’ll leave it at your discretion whether you want to leave this and #74 open, or close them till we make a call (hopefully soon!).

[chriskrycho]

@simonihmig any interest in rebasing this and landing it, at least as a stopgap while we iterate toward something better? Making sure our output type-checks seems like a big win. 😅

[chriskrycho]

I like having this in place; let's rename it to test.tsconfig.json so that it doesn't get picked up on the editor side. We may want to actually migrate this repo to TS, so we need the test-specific config to be separate!

[chriskrycho]

Per comment on the tsconfig.json file, we should reference the test config instead. Also there's a typo here: should be --noEmit.

Suggested change

	"lint:blueprints": "tsc -noEmit",
	"lint:blueprints": "tsc test.tsconfig.json --noEmit",

[tdwesten]

@simonihmig please, take a look at the comments form @chriskrycho!

[simonihmig]

Sorry folks, somehow I missed the previous comments... (probably because they were posted on my birthday, and I had better things to do 😉)

I just noticed that emberjs/rfcs#776 has just been merged, so we now have a way better path forward to support TS blueprints. Therefore I see little value in investing more time into this. @chriskrycho do you agree?

[chriskrycho]

@simonihmig I actually think it’s still very valuable, and I also have time to shepherd it across the line if you have time to rebase it/get it up to date. The reason it’s still valuable: while we have the ability to author blueprints in TS, we still have some design work to do in terms of what the content of those blueprints should be. That’s relatively small, and I will have the RFC written this quarter—probably in February, but we even if it’s the easiest turn-around ever on an RFC, and we land the blueprints implementations the second it merges, that would put it landing in stable Ember CLI 4.4 in June at the earliest. Soooooo if you have time, let’s get this done!

[simonihmig]

@chriskrycho I rebased this, applied your suggestions, and updated the related dependencies (e.g. latest TS).

Also added the linting step to CI, which is for now allowed to fail, as there are expectedly a bunch of typing errors in the existing blueprint files.

[chriskrycho]

Excellent. I'll re-review on Monday! Thank you!

[simonihmig]

Excellent. I'll re-review on Monday! Thank you!

@chriskrycho ping 😉

[chriskrycho]

Just found this while finally (!) clearing out email. Let’s wait till we land #240 to avoid blocking that. Going to try to make that happen early this next week. Thanks for the ping!

[muziejus]

I can give a preview of what will happen once #240 lands.

Broadly, the errors fall into these categories:

• Properties missing on TestContext, including:
  • appInstance Fix: Get rid of pre-rfc232 blueprints.
  • application
  • instance
  • subject (pre-rfc232 files) Fix: Get rid of pre-rfc232 blueprints.
  • TestApplication
  • $ (assert.strictEqual(this.$().text().trim(), '1234');) Fix: Get rid of pre-rfc232 blueprints.
• Properties missing on Hooks, including:
  • appInstance Fix: Get rid of pre-rfc232 blueprints.
  • application Fix: Get rid of pre-rfc232 blueprints.
• Issues with type checks in DOM-related assertions, such as:
  • Argument of type 'Element | Document' is not assignable to parameter of type 'string | Element | null | undefined'. Fix: replace assert.dom(this.element) with assert.dom().
  • Object is possibly 'null'. wrt element in this.element.textContent.trim(). Fix: replace with this.element?.textContent?.trim()
• Missing exported members, including:
  • moduleFor in ember-qunit (pre-rfc232 files) Fix: Get rid of pre-rfc232 blueprints.
• Missing modules with relative pathnames, including:
  • ../../helpers/destroy-app
  • ../../tests/helpers/module-for-acceptance Fix: Get rid of pre-rfc232 blueprints.
  • ../transforms/custom-transform
• This specific problem:
  • let component = this.owner.factoryFor('component:x-foo').create(); causes Object is of type 'unknown' on factoryFor Fix: Get rid of unit test blueprints for components.
• Unused variable:
  • 'ApplicationInstance' is declared but its value is never read.
• General errors with @attr and transforms.

By far and away the most common errors are the two pre-rfc232 errors, looking for moduleFor and having no subject property on TestContext. Given that rfc232 was implemented w/ ember 3.0 (if my reading is right), I think we should just not have .ts blueprints for the pre-rfc232 test files.

I've solved some of the problems already and will create a new PR with the updated fixtures and blueprints hopefully before this one lands. 14 files changed, three or so added. Here's what I've solved:

• declared but unused appInstance param in instance-initializer fixtures. Solved by commenting out the param, however now there is an error of ApplicationInstance being declared but unused.
• Errors regarding store in ember-data tests. I solved this by doing let store = this.owner.lookup('service:store') as Store; and importing the Store in the test file. Not a big lift. Solved by adding as Store at end of lookups.
• However, when I do that, in the this.store.createRecord() call in the next line of the test blueprint, I get this error: Argument of type 'string' is not assignable to parameter of type 'never', as createRecord() is expecting: DS.Store.createRecord<K extends never>(modelName: K, inputProperties?: {}): ModelRegistry[K]. This is not what I get in my app, where <K extends ModelRegistry>. Solved by adding a mock ModelRegistry and a mock SerializerRegstry to new file types/ember-data.d.ts.
• No type given to params in the route-with-dynamic-segment fixture. Solved by changing model(params) to model(params: {}).
• Problems loading related models in the model fixtures. Solved by making dummy comment.d.ts, post.d.ts, and user.d.ts in node-tests/fixtures/model-test/.
• issues with instance and application, etc., in instance initializer tests Solved by typing Application and ApplicationInstance in test fixtures.
• Can't find my-app/config/environment in instance-initializer test. Solved by declaring my-app/config/environment module in types/index.d.ts.
• /node-tests/fixtures/mu-helper.ts has implicit any for positional Solved by typing positional: {}.
• assert.dom() is an unknown property on assert. Solved by updating qunit-dom and adding import 'qunit-dom' to types/ember-data.d.ts`.
• And application variable issues in node-tests/fixtures/acceptance-test/mocha.ts Solved by typing as Application and importing Application from @ember/application.
• Can't find type declarations for @glimmer/component Solved by adding @glimmer/component to dev dependencies.

Here are the remaining complaints:

Found 37 errors in 10 files.

Errors  Files
     6  node-tests/fixtures/initializer-test/rfc232-dummy.ts:11
     6  node-tests/fixtures/initializer-test/rfc232.ts:11
     1  node-tests/fixtures/initializer/initializer-nested.ts:1
     1  node-tests/fixtures/initializer/initializer.ts:1
     1  node-tests/fixtures/instance-initializer-test/mocha.ts:6
     9  node-tests/fixtures/instance-initializer-test/rfc232-dummy.ts:11
     9  node-tests/fixtures/instance-initializer-test/rfc232.ts:11
     1  node-tests/fixtures/instance-initializer/instance-initializer-nested.ts:1
     1  node-tests/fixtures/instance-initializer/instance-initializer.ts:1
     2  node-tests/fixtures/model-test/octane-foo-attrs.ts:2

[muziejus]

@simonihmig I have been testing this PR against #240, and I've had to add the following dev dependencies in addition to what you have above:

• @glimmer/component
• @types/ember-data
• @types/ember-data__adapter
• @types/ember-data__model
• @types/ember-data__serializer
• @types/ember-data__store

I also had to upgrade qunit-dom to 2.0.0 to get types to appear.

[simonihmig]

Ok, thanks for the heads up @muziejus! I'll wait for #240 to land before updating this again!

[muziejus]

Thanks, @simonihmig! I think for @chriskrycho's sake, who's had to review #240 like five times now, I'll have that land and then follow up w/ a PR that adds types to the fixtures and adjusts the blueprints to match them--but that doesn't actually have any mechanism for doing the type checks.

Then, I think, once that second PR lands, we can implement the type-checking mechanism you've put together here, so that yarn lint:blueprints will only be implemented once the types are in place.

That makes sense to me, how does it sound to you?