Skip to content

fix(rest): use direct import to work around a TS bug #1689

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2018
Merged

fix(rest): use direct import to work around a TS bug #1689

merged 1 commit into from
Sep 10, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Sep 9, 2018 •
edited
Loading

See microsoft/TypeScript#26985

Work around #1688 (comment)

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner September 9, 2018 04:15
@raymondfeng raymondfeng mentioned this pull request Sep 9, 2018
Closed
@marioestradarosa
Copy link
Contributor

marioestradarosa commented Sep 9, 2018 •
edited
Loading

@raymondfeng , the current lb4 app scaffolds the app correctly with ServiceMixin, however it needs service-proxy": "^0.8.0 but it writes currently service-proxy": "^0.7.1.

I noticed that the package.json for CLI under templateDependencies is correct 0.8.0 but wasn't published to NPM , it was updated the next day.

Is that the reason why we are experiencing this effect ? 0.7.1 instead of 0.8.0

@marioestradarosa
Copy link
Contributor

I see this PR fixes the other issue with openapi3-ts. but please check my other comment, I think based on the first issue opened, I could see two problems.

@raymondfeng
Copy link
Contributor Author

I suspect that one of the publish failed half way and left the versions inconsistent

bajtos
bajtos suggested changes Sep 10, 2018
View reviewed changes
packages/rest/src/keys.ts Outdated
* See https://github.com/Microsoft/TypeScript/issues/26985
*/
// import {OpenApiSpec} from '@loopback/openapi-v3-types';
import {OpenAPIObject} from 'openapi3-ts';
Copy link
Member

Choose a reason for hiding this comment

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

Can you please rename OpenAPIObject to OpenApiSpec to avoid changing the places using this type below? We don't want to rename them from OpenApiSpec to OpenAPIObject now and then rename them back to OpenApiSpec when the typescript bug is fixed.

import {OpenAPIObject as OpenApiSpec} from 'openapi3-ts';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@raymondfeng raymondfeng force-pushed the workaround-typescript-26985 branch from dfab96d to a77970c Compare September 10, 2018 14:34
@raymondfeng raymondfeng force-pushed the workaround-typescript-26985 branch from a77970c to 95cf976 Compare September 10, 2018 14:40
@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

b-admike
b-admike approved these changes Sep 10, 2018
View reviewed changes
packages/rest/src/keys.ts
/**
* See https://github.com/Microsoft/TypeScript/issues/26985
*/
// import {OpenApiSpec} from '@loopback/openapi-v3-types';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this import statement?

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 intentionally kept it here so that we don't forget to revert it once the TS issue is fixed.

bajtos
bajtos approved these changes Sep 10, 2018
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

:shipit:

@raymondfeng raymondfeng merged commit 2cf3b2c into master Sep 10, 2018
@bajtos bajtos deleted the workaround-typescript-26985 branch September 10, 2018 16:08
@marioestradarosa marioestradarosa mentioned this pull request Sep 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos approved these changes

@b-admike b-admike b-admike approved these changes

Assignees
No one assigned
Labels
critical
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@raymondfeng @marioestradarosa @bajtos @b-admike