PoshCode / ModuleBuilder

A PowerShell Module to help scripters write, version, sign, package, and publish.
MIT License
445 stars 54 forks source link

Export alias create with Set-Alias and New-Alias #103

Closed johlju closed 2 years ago

johlju commented 3 years ago

ModuleBuilder is automatically exporting aliased in the module manifest if they are decorated with the attribute [Alias()]. The function that finds the alias to export is GetCommandAlias.

I suggest we extend the function GetCommandAlias to search for Set-Alias as well.

Set-Alias -Name 'Task.Generate_Conceptual_Help' -Value "$PSScriptRoot/tasks/Generate_Conceptual_Help.build.ps1"

This will help when using the pattern with separate file for each alias in the Public folder, like Task.Generate_Conceptual_Help.ps1. We should make sure to only parse script files in the folder Public to avoid the Set-Alias is also used for private functions which should not be exported.

The function GetCommandAlias here should be updated:

https://github.com/PoshCode/ModuleBuilder/blob/abb0e09297d55e6c9e3f519b7e177eae1da7f3d9/Source/Private/GetCommandAlias.ps1#L1-L23

johlju commented 3 years ago

@Jaykul what do you think of this suggestion? Happy to work on it if you feel it suits this project.

Jaykul commented 3 years ago

Can you explain why we would want to ship aliases to external files, instead of putting the commands into the module properly? That sounds like an anti-pattern...

Jaykul commented 3 years ago

Conceptually, I don't mind including aliases set by Set-Alias, but it's complicated to get right:

  1. Discover aliases from New-Alias
  2. Discover aliases from Set-Alias?
  3. Avoid aliases that are defined within functions and not set explicitly to script scope
  4. Avoid aliases that are defined in script scope, but are explicitly set or created in a different scope
  5. Avoid aliases that are removed later? ...

Hmm, maybe you could limit only to aliases that are explicitly in an Export-ModuleMember statement, or do we need to generate one of those, too? Aliases defined with New or Set aren't exported from the psm1 by default, are they?

johlju commented 3 years ago

Can you explain why we would want to ship aliases to external files, instead of putting the commands into the module properly? That sounds like an anti-pattern...

@Jaykul this is a pattern for Invoke-Build for it access build tasks. Build tasks are in a separate script file outside of the module, for example:

:DSCRESOURCE.DOCGENERATOR\0.8.0
|   DscResource.DocGenerator.psd1
|   DscResource.DocGenerator.psm1
|
\---tasks
        Generate_Conceptual_Help.build.ps1
        Generate_Wiki_Content.build.ps1
        Publish_GitHub_Wiki_Content.build.ps1

This is documented here nightroman/Invoke-Build: example-2-import-from-a-module-with-tasks

Inside the module there is several Set-Alias (or New-Alias) that point to each build script (shown in the issues description).

It is possible to looping through each build task script in the suffix.ps1 for example https://github.com/gaelcolas/Sampler/blob/master/Sampler/suffix.ps1. But having module builder actually export these would be better as there are no need to do this initial step when the module is imported.

johlju commented 3 years ago

Conceptually, I don't mind including aliases set by Set-Alias, but it's complicated to get right:

  1. Discover aliases from New-Alias
  2. Discover aliases from Set-Alias?
  3. Avoid aliases that are defined within functions and not set explicitly to script scope
  4. Avoid aliases that are defined in script scope, but are explicitly set or created in a different scope
  5. Avoid aliases that are removed later?

My though was to have this flow:

  1. Parse all *.ps1 files in folder Public for Set-Alias and New-Alias (exclude PublicFilter).
  2. Check the parent AST so they are at the script level of the .ps1 (and not part of a function).

The above cover your number 1-3. I'm not seeing number 4 as being a problem as we should only look at the script level and ignore anything else? With number 5 I'm guessing you mean if a script has Remove-Alias. 🤔 That is an edge case... Like someone adding [Alias()] and in another script file does `Remove-Alias. 🤔 Maybe that alias shouldn't been used to begin with then? 🤔

Hmm, maybe you could limit only to aliases that are explicitly in an Export-ModuleMember statement, or do we need to generate one of those, too?

No need to generate those, and I think we should avoid having Export-ModuleMember in the source file which then gets added to the built module .psm1? 🤔 Then behavior would be different depending if importing the .psd1 or .psm1 directly.

Aliases defined with New or Set aren't exported from the psm1 by default, are they?

Not that I have seen no. But probably happens if importing a .psm1 directly that does not have an Export-ModuleMember.

johlju commented 3 years ago

@Jaykul I sent in draft PR https://github.com/PoshCode/ModuleBuilder/pull/104, before I start fixing tests, if you have time, could you look over it and see if it is as you think 🙂 If it looks good I will fix the tests.

Jaykul commented 3 years ago

Ok, so --despite how fantastically it works-- pretty much everything about how Invoke-Build is built is an anti-pattern, and I want to actively discourage anyone from building modules like that. Invoke-Build isn't a module, and it doesn't export any commands, which means you get no "intellisense" help from editors when you're authoring tasks and other things in that ecosystem.

After a quick test, I believe all aliases are exported from the psm1 by default, so we do not need to generate an Export-ModuleMember to make them available. If it's present, it does prevent aliases from being available by default though...

johlju commented 2 years ago

With the improved changes in PR https://github.com/PoshCode/ModuleBuilder/pull/116 this will be fixed. I close this issue, since the PR is about to be merged.