Skip to content

Exactly what are you trying to do in your auto-generated modules? #372

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

Open
KirkMunro opened this issue Sep 14, 2020 · 0 comments
Open

Exactly what are you trying to do in your auto-generated modules? #372

KirkMunro opened this issue Sep 14, 2020 · 0 comments

Comments

@KirkMunro
Copy link

KirkMunro commented Sep 14, 2020

Discover of issue #371 led to me digging into your module internals more, and I think you need to re-think what you are doing with your auto-generated modules.

Let's take Microsoft.Graph.Users.User version 0.9.1 as an example.

It contains a top-level module that is defined using a psd1 file, a dll, and multiple psm1 files.

The psd1 file identifies the psm1 file as the root module, but it does not identify the dll as a nested module to load as part of the NestedModules entry in the manifest. Instead it leaves the psm1 file to manually load the dll.

Issue: There is no need to manually load a nested binary module like this.
Recommendation: Simply add the relative path (e.g. './bin/Microsoft.Graph.Users.User.private.dll') to the NestedModules entry in the manifest, and PowerShell will load it for you. That's what NestedModules is for!

Once the binary module is manually loaded, the main psm1 file loads a custom psm1 script module that in turn loads an internal psm1 script module. The custom psm1 script module then goes on to disable implicit exports, and then dot source any nested ps1 files that are in the custom folder.

Issue: You don't have any nested ps1 files in custom folders in any of the Graph modules. None.
Recommendation: Maybe this is some legacy code? It's just more processing for nothing though, so if it's legacy, remove that logic. If not, comment it out until you need it. No need to do file I/O where it is not required.

The custom psm1 file then goes on to export the functions and aliases that are defined at that point, which will make them visible to the parent module.

The internal psm1 file switches the contents of a subfolder based on the Graph profile that was selected, and then reads all ps1 files in that profile subfolder to get the command definitions for that module. It then exports the functions and aliases, which will make them visible to the parent module.

Going back to the top-level psm1 file, it also reads the profile settings and loads the cmdlets from the exports path.

Issue: Why does it take three psm1 files to do what one psm1 file could do for you? There doesn't seem to be any need for your custom psm1 file, nor does there seem to be any need for your internal psm1 file. Your top-level psm1 file can do (and is already doing!) what is needed without the others adding extra work to the loading of these modules. Sure enough, if I look at the information stream output, I see multiple duplicate messages that reinforce that there is unnecessary duplication being done here.
Recommendation: Reel this back in to just one psm1 file, one psd1 file, and one dll, to keep things performant and eliminate duplication of effort. You don't need more than a single psm1 file with what you have today. Auto-generated code can and should be clean.

Other miscellaneous issues:

  • You're using pipelines in your psm1 files where they are not necessary. Pipelines add overhead to processing times. They should be used when processing large amounts of data that you don't have in memory. They should not be used when you have data already in memory. Examples taken from Microsoft.Graph.Users.User.psm1:

    Instead of this Do this Reason
    $directories = Get-ChildItem -Directory -Path $exportsPath $directories = @(Get-ChildItem -Directory -Path $exportsPath)` This is necessary for the changes recommended below
    if(($directories | ForEach-Object { $_.Name }) -contains $instance.ProfileName) { if($directories.foreach('Name') -contains $instance.ProfileName) { Collection is already in memory (performance), code is easier to maintain (simplicity)
    $profileDirectory = $directories | Where-Object { $_.Name -eq $instance.ProfileName } $profileDirectory = $directories.where{$_.Name -eq $instance.ProfileName} Performance and simplicity, as above
    } elseif(($directories | Measure-Object).Count -gt 0) { } elseif ($directories.Count -gt 0) { No pipeline (performance) and and simplicity again
    $profileDirectory = $directories | Select-Object -Last 1 $profileDirectory = $directories[-1] No pipeline (performance) and simplicity again
    Get-ChildItem -Path $exportsPath -Recurse -Include '*.ps1' -File | ForEach-Object { . $_.FullName } `foreach ($file in Get-ChildItem -Path $exportsPath -Recurse -Include '*.ps1' -File) {. $_.FullName } Collection is small, faster to process using foreach keyword (performance)

Last, in your psd1 file you are still using wildcards for AliasesToExport. When a single module does hit, it results in a performance hit for all of PowerShell because command discovery is affected. Please, please replace AliasesToExport with the actual list of aliases that are exported, or an empty array if there are none. Ditto for FunctionsToExport and CmdletsToExport.
AB#7392

@ghost ghost added the ToTriage label Sep 14, 2020
@MIchaelMainer MIchaelMainer added this to the Post GA milestone Sep 17, 2020
@ddyett ddyett added the promote label Jan 11, 2021
@peombwa peombwa removed the promote label Jul 15, 2021
@peombwa peombwa removed this from the Post GA milestone Jul 15, 2021
@georgend georgend linked a pull request Jul 15, 2021 that will close this issue
@georgend georgend removed a link to a pull request Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants