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

Replace deprectated importsNotUsedAsValues option with verbatimModuleSyntax #2512

Merged
merged 16 commits into from
Jan 16, 2025

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Jan 10, 2025

Pull Request

🤨 Rationale

The importsNotUsedAsValues TypeScript option is deprectated and will stop working in TypeScript 5.5. It is replaced by verbatimModuleSyntax.

My motivation for this change is that while updating Nimble to Angular 18, I found that importsNotUsedAsValues was forcing me to do a type-only import of Router and ActivatedRoute in router_link.ts. But doing so caused a different error for NimbleAnchorRouterLinkWithHrefDirective:

error NG2016: The directive NimbleAnchorRouterLinkWithHrefDirective inherits its constructor from RouterLink, but the latter has a constructor parameter that is not compatible with dependency injection. Either add an explicit constructor to NimbleAnchorRouterLinkWithHrefDirective or change RouterLink's constructor to use parameters that are valid for DI.

Both Router and ActivatedRoute are parameters to the RouterLink constructor. The problem is that if we do a type-only import of Router and ActivatedRoute, the compiler doesn't know that they are decorated to be injectable. Using verbatimModuleSyntax solves this by allowing us to import from the definition file rather than the type declaration file for those types, even though we're not directly using the definition.

This PR changes the option for the Angular projects, as well as nimble-components, spright-components, and storybook.

👩‍💻 Implementation

Had to update all imports of type symbols to explicitly say type.

🧪 Testing

Builds and existing tests pass.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc
Copy link
Contributor Author

m-akinc commented Jan 10, 2025

@mollykreis will you buddy this change?

@m-akinc m-akinc requested a review from mollykreis January 10, 2025 00:23
@m-akinc m-akinc requested a review from rajsite January 10, 2025 23:33
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

PR is in draft and build is failing but I was reset. I'm assuming still in dev and I'm not reviewing yet but FYI in-case a review was expected.

@m-akinc m-akinc changed the title Replace deprectated importsNotUsedAsValues option with verbatimModuleSyntax in Angular projects Replace deprectated importsNotUsedAsValues option with verbatimModuleSyntax Jan 15, 2025
@m-akinc m-akinc requested a review from rajsite January 15, 2025 19:30
@m-akinc m-akinc marked this pull request as ready for review January 15, 2025 19:31
@m-akinc m-akinc requested a review from jattasNI January 16, 2025 19:36
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

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

I don't think you need to wait for any of the other code owners to submit this.

@m-akinc
Copy link
Contributor Author

m-akinc commented Jan 16, 2025

There were three tab scrolling tests failing in Webkit, but I suspect they must be intermittent. I've restarted the build, and I'll disable those tests in a separate PR, assuming my change isn't somehow responsible.

@m-akinc m-akinc enabled auto-merge (squash) January 16, 2025 19:41
@m-akinc m-akinc disabled auto-merge January 16, 2025 19:51
@m-akinc m-akinc requested a review from mollykreis January 16, 2025 22:31
@m-akinc m-akinc enabled auto-merge (squash) January 16, 2025 22:32
@m-akinc m-akinc merged commit 8b17b15 into main Jan 16, 2025
12 checks passed
@m-akinc m-akinc deleted the users/makinc/use-verbatimmodulesyntax branch January 16, 2025 22:47
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.

5 participants