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

[Track2] Create ServiceClient failed: Class constructor ServiceClient cannot be invoked without 'new' #1004

Closed
dw511214992 opened this issue Jun 2, 2021 · 29 comments
Assignees
Milestone

Comments

@dw511214992
Copy link
Member

reported by @colawwj
image

@sarangan12
Copy link
Member

sarangan12 commented Jun 8, 2021

I have analyzed the issue and am able to reproduce the issue. This issue happens only in the following case: With the new V6 Generator AND core V2 Libraries.

Why?

  1. Before Core V2 Libraries, all our generated libraries use core-http library. This core-http library is built with a tsconfig.json which states that the "target": "es5".
  2. But, with Core V2 Library, all our generated libraries use core-client and core-rest-pipeline libraries.

The core-client library is built with a tsconfig.json which states that "target": "ES2015".
The core-rest-pipeline library is build with a tsconfig.json which states that "target": "ES2015".

  1. All our generated libraries state that the "target": "es5". For example, You can look at the tsconfig.json of the Management Compute Library(reported in this issue) could be found here.

As you can see above, the generated libraries are built with es5 when the underlying dependency of core-client and core-rest-pipeline packages are built with es2015. They are not compatible and causes the reported issue.

How to solve the issue?
Option 1
We could change the target of core-client and core-rest-pipeline packages to es5 and release them. Then, use the released versions of core-client and core-rest-pipeline packages in the SDK Generator. This will resolve the issue.

Option 2
We could change the target of our generated libraries to es2015/es6. This will also resolve the issue. In addition, this will be in adherence to the prescribed guidelines here. @dw511214992 what is your opinion on changing the target to es2015/es6 and drop support for es5? Could you share your thoughts? What kind of impact/response do you think this will have on our customers?

@jeremymeng @xirzec I wanted to ask you if there is any specific reason that the core-client and core-rest-pipeline packages have the target of es6 (which is different from the target es5 of core-http package). Also, what is your opinion on the above options? Any recommendations would be helpful. (I want to specifically point out that if these decision will impact Azure/azure-sdk-for-js#15545)

@dw511214992 I will get Jeff, Jeremy and your opinion and decide on the final option and implement it. In the mean time, you could manually change the target of the generated libraries to es6 and continue with your testing. This would unblock you temporarily in your trying out of the new SDK generator. Thanks.

@ramya-rao-a FYI....

@dw511214992
Copy link
Member Author

dw511214992 commented Jun 8, 2021

Hi @sarangan12 In Option 1, you say we could change the target of core-client and core-rest-pipeline packages to es2015. Here, should es2015 be changed to es5?
About the propoesed options, I prefer set target of core-client and core-rest-pipeline packages to es5 becasue I'm worrying that some customers' code is based on es5 and may encounter error when our package is using es6. (I'm not sure whether there is an issue here. Please correct me if I'm wrong.)
Thanks

@ffMathy
Copy link

ffMathy commented Jun 8, 2021

Pretty sure webpack includes babel, which will transpile for most customers, even if picking the es2015.

@sarangan12
Copy link
Member

@dw511214992 Yes. Option 1 is to change the target of core-client and core-rest-pipeline to es5.

@sarangan12
Copy link
Member

@ffMathy This change would impact both node and web scenarios. So, though dropping the support for es5 will not cause a big impact for web users (because of babel), the node only customers may still encounter an error.

@ffMathy
Copy link

ffMathy commented Jun 8, 2021

@sarangan12 people also use webpack for node projects. It's very rare that webpack is not used.

@sarangan12
Copy link
Member

@ffMathy Agreed. But, I still think we cannot make a design decision based on the assumption that the customers would be using something external.

@jeremymeng
Copy link
Member

@jeremymeng @xirzec I wanted to ask you if there is any specific reason that the core-client and core-rest-pipeline packages have the target of es6 (which is different from the target es5 of core-http package).

I believe we target es5 in core-http because we need to support IE11 for storage.

@xirzec
Copy link
Member

xirzec commented Jun 8, 2021

Yes, the only reason we used es5 before was to support IE11, which hopefully will be a non-goal soon.

So, I think our generated libraries should update their target to something more modern and in-line with our support matrix.

@ffMathy
Copy link

ffMathy commented Jun 10, 2021

Any timeframe on this? And is there a workaround while we wait?

@sarangan12
Copy link
Member

@ffMathy I am working on it in #1027. It should be released before 06/15.

@ffMathy
Copy link

ffMathy commented Jun 10, 2021

Awesome @sarangan12 - you're the best! ❤️

@sarangan12 sarangan12 self-assigned this Jun 10, 2021
@sarangan12 sarangan12 added this to the [2021] July milestone Jun 10, 2021
@sarangan12
Copy link
Member

The PR #1027 has been merged. Code changes will be released in the next preview (which will be released before 06/15)

@ffMathy
Copy link

ffMathy commented Jun 10, 2021

@sarangan12 great news, but how would we use the preview from the commandline? 🙏

@DavidNorena
Copy link

is there a way we can test this before preview ?

@sarangan12
Copy link
Member

@ffMathy After release if you use autorest --reset command, the local cache should clear and next time it should pick the latest preview version. Alternatively, you could use --use=@autorest/[email protected] in the autorest command to pick a specific version.

@DavidNorena
Copy link

sarangan12 the beta version doesnt exists yet, or am I missing something ?

@sarangan12
Copy link
Member

@DavidNorena It is 'After the release'. The beta.3 version has not been released yet. It will be released on Tuesday

@ffMathy
Copy link

ffMathy commented Jun 15, 2021

@sarangan12 great! What is with the versioning though? Why is the alpha version 6.0.0? I'd assume it was a later version than the beta.

@ramya-rao-a
Copy link
Contributor

Good question @ffMathy!

The latest code generator goes by a new package name @autorest/typescript while the older one was @microsoft.azure/autorest.typescript which has v4 as the latest version. v5 for the latter is still in "preview" and is used internally for some of the Azure packages which will move off of it soon. We went with v6 for the newer code generator so that it is easy for us to talk in terms of v4, v5, v6. So you see v6 alpha.

But there was another argument to be made that since it is a whole new package, it should be ok to start from v1. So you see v1 beta.

We will finalize the versions soon, but am curious what your thoughts on the matter are :)
@DavidNorena, feel free to comment as well

@ffMathy
Copy link

ffMathy commented Jun 15, 2021

Well I suppose it's fine if you clearly deprecate the old package and link to the new one.

If you don't though, its version should be 7 in my opinion.

@ffMathy
Copy link

ffMathy commented Jun 16, 2021

Also @ramya-rao-a, if the new package started at 1.0, why does NPM list the 6.0 versions too?
image

That's just even more confusing. In fact, I believe installing the package now with NPM would result in it fetching 6.0, because it it a later version.

@deyaaeldeen
Copy link
Member

@ffMathy you do not need to install this package manually at all. Autorest will download whatever is tagged with latest on npm when you ask for the typescript code generator. You can confirm this by running the autorest command and inspect the printed log on the console for versions but make sure to not specify any versions in the arguments.

That being said, I see your point that this could be confusing when looking at the npm website. @ramya-rao-a do you think it is ok to unpublish those alphas?

@ffMathy
Copy link

ffMathy commented Jun 17, 2021

Alright. I agree with unpublishing though.

Also, what about the tags here? https://github.com/Azure/autorest.typescript/releases

It says a 6.0.0 version was released relatively recently. That's super confusing as well. It also made me wonder if 6.0 was the active version.

@ramya-rao-a
Copy link
Contributor

Thanks for all the feedback @ffMathy!

We had another round of internal discussions and have decided to go back to using v6 to avoid the confusion.
Logged #1041 to track the next steps here

@ffMathy
Copy link

ffMathy commented Jun 18, 2021

@ramya-rao-a you are very welcome!

And great decision! I think that makes everything easier. Cheers!

@ffMathy
Copy link

ffMathy commented Jun 19, 2021

The latest "beta 3" release did not solve this for me.

Here's the log output, proving it did in fact use the package.

Cleared the AutoRest extension folder.
On the next run, extensions will be reacquired from the repository.
AutoRest code generation utility [cli version: 3.2.1; node: v14.16.1, max-memory: 4096 MB]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest

There is a new version of AutoRest available (3.2.3).
 > You can install the newer version with with npm install -g autorest@latest

   Loading AutoRest core      'C:\Users\mathi\.autorest\@[email protected]\node_modules\@autorest\core\dist' (3.4.5)
INFORMATION: > Installing AutoRest extension '@autorest/typescript' (latest)
INFORMATION: > Installed AutoRest extension '@autorest/typescript' (latest->1.0.0-beta.3)
INFORMATION: > Installing AutoRest extension '@autorest/modelerfour' (4.15.456)
INFORMATION: > Installed AutoRest extension '@autorest/modelerfour' (4.15.456->4.15.456)

I get the same error at runtime.
image

@ffMathy
Copy link

ffMathy commented Jun 19, 2021

I can also verify that the package.json of my client is indeed es6, but it doesn't solve the issue for me.

Can we re-open the issue?

@sarangan12
Copy link
Member

@ffMathy

I have just verified it from scratch and can confirm the issue no longer happens. Let us go step by step:

  1. Issue the command autorest --reset in command line
  2. Now, regenerate your package (In your case, I am assuming the name is openapi) using the following command sample:
autorest --typescript --use=@autorest/[email protected] --input-file=C:\Users\sarajama\Projects\autorest.typescript\test\integration\swaggers\license-header.json --title=NoLicenseHeaderClient --package-name=nolicense-header --output-folder=C:\Users\sarajama\Projects\autorest.typescript\test\integration\generated\noLicenseHeader --license-header=false --allow-insecure-connection=true
  1. Now, go to the generated package. Confirm the target is set to es6 in the tsconfig.json file.
  2. Issue the following commands: npm install && npm run build && npm pack. You will get the packaged file for your openapi project.
  3. Import it in your sample project. Build it & run it.

I have followed the exact same steps and can confirm the issue does not happen. Would you mind trying it again and let me know if the issue still persists.

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

No branches or pull requests

8 participants