OfficeDev / generator-office

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

Adding shared template option #717

Closed igor-ribeiiro closed 2 years ago

igor-ribeiiro commented 2 years ago

Thank you for your pull request. Please provide the following information.


  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)
    • [X] Yes
    • [ ] No

There will be a new option for the shared runtime template.

  1. Do these changes impact documentation? (e.g., a tutorial on https://docs.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins)
    • [X] Yes
    • [ ] No

The tutorial on how to create CFs need to be updated. It also needs to include shared vs legacy runtimes differences.

  1. Validation/testing performed:

Tested creating a new yo office app and choosing the option shared template. The template is indeed the shared one.

  1. Platforms tested:

    • [X] Windows
    • [ ] Mac
igor-ribeiiro commented 2 years ago

Added the shared option before the manifest option and reordered the options for most used to least used. The change https://github.com/OfficeDev/generator-office/pull/714/files caused a bug which the message wouldn't have the host in it: image

So I also fixed that bug.

igor-ribeiiro commented 2 years ago

@millerds "I'm not seeing that in the documentation and if it really didn't have the default answer then wouldn't there be an error when trying to create the project since there wouldn't be an answer (i.e. something like a null exception)?"

It is null that gets returned (or something close to it). We don't get an error because we check if it exists:

        scriptType: answerForScriptType.scriptType ? answerForScriptType.scriptType : this.options.ts ? typescript : javascript

The default is only the value that will get chosen when you press "enter", without choosing a particular option.

millerds commented 2 years ago

So if that line is taking care of the question not asked case, then there is no need for that if block. It looks like the bug you are saying you are fixing is not with the language chosen, but rather the host (that is what is missing from the message). The host also has a fall back in the same place as the language, but that isn't using the ?: case but rather the || syntax and probably behaves differently.

The point is that we shouldn't need two places to fall back onto and I prefer the _configureProject method with a simple value precedent check rather than having to maintain multiple conditionals with the same tests (the one for weather or not the question should be asked, and the same one for weather or not the question was asked.

igor-ribeiiro commented 2 years ago

Member

Sure, we can handle all these cases handling at the same place. I made some changes so that all cases are covered in a single line and hopefully it is not too confusing.

igor-ribeiiro commented 2 years ago

@millerds could you review it again? Needed to comment some SSO tests that were added back during the merge