ember-cli / ember-page-title

Page title management for Ember.js Apps
https://ember-cli.github.io/ember-page-title/
Other
188 stars 57 forks source link

feat: typescript #246

Closed knownasilya closed 10 months ago

knownasilya commented 2 years ago

Closes https://github.com/ember-cli/ember-page-title/issues/239

knownasilya commented 2 years ago

I think this is ready?

chriskrycho commented 1 year ago

I'll take a look this week!

chriskrycho commented 1 year ago

@knownasilya can you poke at the failing CI stuff?

chriskrycho commented 1 year ago

@knownasilya let me know when you've addressed everything and are ready for a re-review. 😅

esbanarango commented 1 year ago

👀 🙏

knownasilya commented 1 year ago

@esbanarango if you have time to tackle any of the feedback, help is appreciated (just submit prs against this branch)

knownasilya commented 1 year ago

@chriskrycho for some reason the global types I defined in dev-only-types don't apply https://github.com/ember-cli/ember-page-title/actions/runs/5622356609/job/15234865251#step:4:94

leepfrog commented 1 year ago

Opened up #256 that might address some of the typescript issues I was seeing

leepfrog commented 1 year ago

Specific things that are breaking:

knownasilya commented 1 year ago

@leepfrog so we don't support Ember v4 at all? Can we make it work with v4, at least the current supported versions? I don't think we dropped v4 yet and this is an official package.

NullVoxPopuli commented 1 year ago

I don't see anything that wouldn't be compatible with v4

we should maximize compatibility where we can so that folks can be more confident in upgrading

leepfrog commented 1 year ago

The breaking change is this:

https://github.com/ember-cli/ember-page-title/pull/246/commits/3f872327f1aa5c7f46a2acbac17972842c0353a1#diff-9523a096dee8c65f63b50e7dde5065da8b65e38290c8f1cd86cac05442b2fe71L1

- import { getOwner } from '@ember/application';
+ import { getOwner } from '@ember/owner';

I can open a PR against this branch to revert the breaking changes. (Which causes deprecation warnings on ember 5.)

NullVoxPopuli commented 1 year ago

The way to support both old and new without throwing deprecation warnings is to use macros, like here: https://github.com/NullVoxPopuli/ember-resources/blob/cc363c2a17af386be5dc10cd6e3168b9e8c98ece/ember-resources/src/core/function-based/manager.ts#L27-L37 (note, I have no idea what the comment there is saying, so ignore that, not important)

leepfrog commented 1 year ago

TIL! Will use that approach!

leepfrog commented 1 year ago

Opened #257 to handle the conditional import and to add ember 4.8-lts to ember-exam / ci.

knownasilya commented 1 year ago

So new breaking changes are:

I think this sounds right, @leepfrog?

leepfrog commented 1 year ago

Correct!

knownasilya commented 1 year ago

Accidentally borked this branch, going to have to manually merge your commits again @leepfrog 🤦🏼‍♂️

knownasilya commented 1 year ago

@NullVoxPopuli @chriskrycho just waiting on a review to get this across the finish line.

knownasilya commented 1 year ago

@leepfrog I don't have time to address this feedback today. If you have time feel free to do that, otherwise I'll tackle it later this week.

knownasilya commented 1 year ago

@bertdeblock if you have time to address feedback feel free to pr against this branch