autorest.typescript icon indicating copy to clipboard operation
autorest.typescript copied to clipboard

Use shared rollup config when generating rollup files

Open maorleger opened this issue 3 years ago • 13 comments

As you can see here and in the usage of it here we have a shared rollup config that packages should use within the monorepo. This issue tracks updating the generated code to use the same shared config.

I believe https://github.com/Azure/autorest.typescript/blob/main/src/generators/static/rollupConfigFileGenerator.ts needs to be updated here, but I am not familiar with the TS generator to say what else needs updating

maorleger avatar Dec 08 '21 18:12 maorleger

@maorleger thanks for opening this. Currently, code gen generates a rollup config that mirrors what is in the shared one in the mono repo.

Once you merge your PR, feel free to open a PR here against the rollupConfigFileGenerator.ts module with your changes.

deyaaeldeen avatar Dec 08 '21 21:12 deyaaeldeen

@maorleger Am I correct in assuming that you want the rollup to be generated as "https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/formrecognizer/ai-form-recognizer/rollup.config.js"? Could you please confirm?

sarangan12 avatar Dec 09 '21 23:12 sarangan12

@maorleger Am I correct in assuming that you want the rollup to be generated as "https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/formrecognizer/ai-form-recognizer/rollup.config.js"? Could you please confirm?

Yeah, that's what I was thinking originally - would that be a reasonable thing to do here?

maorleger avatar Dec 10 '21 18:12 maorleger

@maorleger, currently code gen does not add dev-tool to the devDependencies list because the code gen smoke tests build generated packages and dev-tool is not published. The current approach is to just duplicate the config file until code gen tests gain the ability to checkout the mono repo and install unpublished deps from there. See https://github.com/Azure/autorest.typescript/pull/954.

deyaaeldeen avatar Dec 10 '21 19:12 deyaaeldeen

@maorleger, currently code gen does not add dev-tool to the devDependencies list because the code gen smoke tests build generated packages and dev-tool is not published. The current approach is to just duplicate the config file until code gen tests gain the ability to checkout the mono repo and install unpublished deps from there. See #954.

Oh thank you for the context here! Was not aware... do you think it makes sense to keep this issue open but on the backburner or just close it? I'm fine either way, as long as the autogen crew had a chance to opine on it 😄

maorleger avatar Dec 10 '21 19:12 maorleger

@maorleger We could do it in other ways too. We could have a flag such as -mono-repo which is set to false by default. (This flag could be used for things such as rollup, tsconfig, rush, etc) So, there will not be any impact to test packages and acual production packages also could be generated easily.

sarangan12 avatar Dec 10 '21 23:12 sarangan12

@sarangan12 I think --mono-repo should be set to true by default, because we don't hope to add --mono-repo in generating codes for azure-sdk-for-js.

dw511214992 avatar Feb 10 '22 05:02 dw511214992

@sarangan12 I'm going to pick this up in March or so (maybe sooner if it's very simple). Do you feel we need to have an isolated (non-monorepo) option for Track 2 generated Azure SDK packages? I could pretty simply update the static generator to depend on dev-tool and use its commands but want to make sure that won't break folks who aren't participating in the rush workspace.

witemple-msft avatar Feb 10 '22 20:02 witemple-msft

By default we could generate for mono repo. We could have an option introduced to follow the non mono repo

sarangan12 avatar Feb 10 '22 23:02 sarangan12

@sarangan12 @witemple-msft I have added a flag --mono-repo in https://github.com/Azure/autorest.typescript/pull/1309 . The flag is set to be true by default, and then we don't need change the command to generate codes for azure-sdk-for-js. In codegen test, we need to add --mono-repo=false in the command.

The PR only contains the changes for RLC, I think you can change the parts of generating regular js sdk. Thanks

dw511214992 avatar Feb 11 '22 07:02 dw511214992

@sarangan12 In @joheredi 's comment, flag azure-sdk-for-js may be preferred. What do you think about it? thanks

dw511214992 avatar Feb 14 '22 01:02 dw511214992

Note: We are continuing the conversation in the PR comment.

sarangan12 avatar Mar 02 '22 19:03 sarangan12

@sarangan12 @joheredi @qiaozha find there is another option is-test-package, and not sure whether it meets this issue's requirement. thanks

dw511214992 avatar Mar 14 '22 08:03 dw511214992