Skip to content

Spike on TS types#992

Closed
gitKrystan wants to merge 4 commits intoemberjs:masterfrom
gitKrystan:ts-spike
Closed

Spike on TS types#992
gitKrystan wants to merge 4 commits intoemberjs:masterfrom
gitKrystan:ts-spike

Conversation

@gitKrystan
Copy link
Contributor

@gitKrystan gitKrystan commented Dec 3, 2022

See #957

This is what would be published:
image

I have lots of questions below.

Requirements before merge

emberjs/ember-test-helpers#1269
emberjs/ember-test-helpers#1270
emberjs/ember-test-helpers#1271
emberjs/ember-test-helpers#1272
emberjs/ember-test-helpers#1277
and ember-source@4.10

Comment on lines +57 to +58
"@glimmer/interfaces": "^0.84.2",
"@glimmer/reference": "^0.84.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK why I had to install these but I had a lot of typescript errors in node_modules without them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that seems very bad. I will check out the PR and see if I can understand why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, after removing the yalc bit, I don't see this. I haven't yet tried it with a yalc'd @ember/test-helpers, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try again now that everything is further along.

@@ -0,0 +1,3 @@
// FIXME: Not sure this actually does anything
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely should do a thing, but it depends on how paths is set I think. This may be basically-invisible as it stands; this is a problem with our blueprint and config that has existed for a very long time.

@@ -0,0 +1,7 @@
// Types for compiled templates
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably remove this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep—in fact we definitely want to! They don't get us anything useful in most apps/addons, but they are completely useless in this one in particular I think.

"ember-load-initializers": "^2.1.2",
"ember-resolver": "^8.0.3",
"ember-source": "~4.9.1",
"ember-source": "^4.10.0-beta.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What needs -beta.2? I assume the Owner types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolver preview types from beta.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I just landed a PR this morning, and will do the back port for it on Monday, which will make it available (along with other Owner types needed) on 4.8 LTS.

@gitKrystan
Copy link
Contributor Author

Superseded by #994

@gitKrystan gitKrystan closed this Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants