Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecations and TS Fixes #746

Merged
merged 15 commits into from
Apr 21, 2023
Merged

Deprecations and TS Fixes #746

merged 15 commits into from
Apr 21, 2023

Conversation

gossi
Copy link
Collaborator

@gossi gossi commented Apr 5, 2023

To prepare a v2.1 release as preparation to upgrade for v3 #744 :

  • Deprecates <Link> component (in favor of (link) helper)
  • Deprecates UILink class (in favor of only having a Link class)
  • Fixes typescript issues Types are wrong - break with TS v4 #742 for type linting

@gossi gossi requested a review from buschtoens April 5, 2023 12:40
Comment on lines 10 to 26
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
@tracked isActive = false;
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
@tracked isActiveWithoutQueryParams = false;
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
@tracked isActiveWithoutModels = false;
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
@tracked isEntering = false;
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
@tracked isExiting = false;
// @typescript-eslint/ban-ts-comment
// @ts-expect-error this fails consumer packages
Copy link
Owner

Choose a reason for hiding this comment

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

I suspect that this will not actually work.

This addon has a build step that is executed before publishing to npm:

ember-link/package.json

Lines 28 to 29 in c8497dc

"prepack": "ember ts:precompile",
"postpack": "ember ts:clean",

This generates the respective .js and .d.ts files.

tsc does not include // @ts-* comments in the resulting .d.ts files:

import Transition from '@ember/routing/-private/transition';
import { Link } from 'ember-link';
export default class TestLink extends Link {
    isActive: boolean;
    isActiveWithoutQueryParams: boolean;
    isActiveWithoutModels: boolean;
    isEntering: boolean;
    isExiting: boolean;
    url: string;
    onTransitionTo?(): void;
    onReplaceWith?(): void;
    get qualifiedRouteName(): string;
    transitionTo(event?: Event): Transition;
    replaceWith(event?: Event): Transition;
    private _preventTransitionOut;
    private _createDummyTransition;
}

So those comments actually have no effect for users.

(FWIW this worked as a patch-package patch in our local repo, because we are depending on an older version that doesn't yet pre-compile.)

Copy link
Owner

Choose a reason for hiding this comment

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

I'll try to think of a backwards-compatible fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point, I forgot about extraction steps in transpilation.

addon/link.ts Outdated Show resolved Hide resolved
addon/components/link/component.ts Outdated Show resolved Hide resolved
@gossi
Copy link
Collaborator Author

gossi commented Apr 7, 2023

Woke up with a solution to the problem we discussed yesterday. Soooo java-ish ain't yet nobody will care 😂

@gossi gossi merged commit 789e620 into master Apr 21, 2023
@gossi gossi deleted the prepare-v2.1 branch April 21, 2023 13:54
@gossi gossi added the internal label Apr 21, 2023
@bertdeblock
Copy link
Contributor

👋 When may we expect the v2.1 release?
I see the deprecation is already cleaned up on main, but AFAICT, it has not yet been released?

@gossi
Copy link
Collaborator Author

gossi commented May 19, 2023

Exactly. I'm traveling and be back next week then I wanna do the release.

Unless ofc @buschtoens can do so and support hotfixes afterwards ;)

@bertdeblock
Copy link
Contributor

Thanks @gossi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants