search

OfficeDev / generator-office

Yeoman generator for building Microsoft Office related projects.
https://www.npmjs.com/package/generator-office
MIT License
825 stars 208 forks source link

Fixing ts option #714

Closed igor-ribeiiro closed 2 years ago

igor-ribeiiro commented 2 years ago

This PR makes the --ts option work again and makes typescript be chosen. It had a bug that the option was getting ignored and the generated project was javascript.

  1. Do these changes impact User Experience? (e.g., how the user interacts with Yo Office and/or the files and folders the user sees in the project that Yo Office creates)

    • [ ] Yes
    • [x] No

    If Yes, briefly describe what is impacted.

  2. Do these changes impact documentation? (e.g., a tutorial on https://docs.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins)

    • [ ] Yes
    • [x] No

    If Yes, briefly describe what is impacted.

  3. Validation/testing performed:

Tested that the yo office --ts options are now creating a typescript project.

  1. Platforms tested:

    • [X] Windows
    • [ ] Mac
millerds commented 2 years ago

So how does this change actually fix the problem?

igor-ribeiiro commented 2 years ago

So how does this change actually fix the problem?

In: https://github.com/OfficeDev/generator-office/blob/master/src/app/index.ts#L302 the answerForScriptType.scriptType is true even if no answer was provided.

This bug was introduced when we wanted to make the preview template option in Yo Office, as it didn't have a language and we didn't want to ask the language question. At that time, we didn't think about the --ts option and its impact on it.

millerds commented 2 years ago

So how does this change actually fix the problem?

In: https://github.com/OfficeDev/generator-office/blob/master/src/app/index.ts#L302 the answerForScriptType.scriptType is true even if no answer was provided.

This bug was introduced when we wanted to make the preview template option in Yo Office, as it didn't have a language and we didn't want to ask the language question. At that time, we didn't think about the --ts option and its impact on it.

So why does that work for the host prompt? Also why is the conditional not needed for the project Name? Also what about the js option?

We should be clear that each of the prompts is behaving correctly. If that change broke one, it could have broken the others as well.

igor-ribeiiro commented 2 years ago

I think I left the host prompt. I will be having a look into fixing it as well as other scenarios

igor-ribeiiro commented 2 years ago

@millerds ready for the PR to be reviewed again

millerds commented 2 years ago

Actually, I went back and looked more closely at that change that broke this and prompt settings and I think this if clauses that were added were unneeded (didn't notice it before). They are being used to set a value when the question wasn't asked, but that should be handled by the "default" property of the prompt object. That is why there is always an answer . . . if the question isn't asked the default value is used and we don't need to use the conditional to change it. We should just remove the conditional.

igor-ribeiiro commented 2 years ago

lw

Yeah, the ifwasn't necessary and I just removed it