Skip to content

Disable .ps1 support in entryPoint #73

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

Closed
daxian-dbw opened this issue Oct 5, 2018 · 15 comments
Closed

Disable .ps1 support in entryPoint #73

daxian-dbw opened this issue Oct 5, 2018 · 15 comments
Assignees
Labels
bug Something isn't working

Comments

@daxian-dbw
Copy link
Contributor

daxian-dbw commented Oct 5, 2018

When working on the refactoring, I found that although Import-Module -Name script.ps1 does import the script file as a module, the module doesn't expose anything, and functions from the script are actually loaded in the global scope.

This unexpected behavior will leave those functions around to pollute the Runspace.
We could use New-Module -ScriptBlock to replace the current approach.
New-Module creates a dynamc module that will be persisted in the session until the session exits. So it doesn't help.

@daxian-dbw daxian-dbw self-assigned this Oct 5, 2018
@daxian-dbw
Copy link
Contributor Author

I guess our choices are:

  • Don't support entrypoint at all.
  • Copy the .ps1 file to a temp file and rename the extension to .psm1, then import-module the .psm1 file. This doesn't seem worth the effort.

I'm inclined to the first option. @tylerl0706 what's your thought?

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Oct 5, 2018

Entrypoint is a Functions concept and I think we should honor it if we can.

cc @asavaritayal @joeyaiello

@TylerLeonhardt
Copy link
Member

If we dot source and then run the function in the local scope, wouldn't that work?

pwsh.AddScript(". ./foo.ps1; Invoke-Foo", useLocalScope: true).Invoke()

@daxian-dbw
Copy link
Contributor Author

Then we cannot use AddCommand, which means we cannot differentiate between a hard exception (the script is broken) and an expected error because all kinds of errors will be written to error stream.

@daxian-dbw
Copy link
Contributor Author

Entrypoint is a Functions concept and I think we should honor it if we can.

I think it depends on whether it adds any value. What would be the scenarios for people to use an entry point along with a .ps1 file? A .ps1 file always has a default entry point.

@TylerLeonhardt
Copy link
Member

I thought we had pwsh.HasErrors available to us? Or is that set to true when there are errors in the error stream?

Also, using AddCommand(...).AddStatement() also has this problem with hard exceptions.

@daxian-dbw
Copy link
Contributor Author

As far as I understand, pwsh.HasErrors will be set to true if there is any error written to the error stream, which means the expected error will make it set to true too.

Didn't know the same issue happens to AddStatement() too. That's a shame 😦

@daxian-dbw daxian-dbw reopened this Oct 8, 2018
@TylerLeonhardt
Copy link
Member

So get this, that actually does throw it looks like?
screenshot_20181008-171704

@TylerLeonhardt
Copy link
Member

Same with dot sourcing actually.

screenshot_20181008-180451

@TylerLeonhardt
Copy link
Member

I might be holding on to nothing here.

Feel free to change it (remove the EntryPoint support) if you think this is something user's won't want and we can get feedback later.

Please confirm with PMs though.

cc @asavaritayal @joeyaiello

@daxian-dbw
Copy link
Contributor Author

Hmm, thanks for verifying it. My memory was a bit off on this one. They have some different behaviors but I couldn't recall exactly what except that AddCommand("NonExistCmd") will throw while AddScript("NonExistCmd") doesn't.

Even with AddScript and dot-sourcing, passing arguments would be awkward. If using @args and AddArguments(..), you are basically assuming the parameter declaration is the in the same order of your passed-in arguments, which is fragile. And again, in my opinion, it's more work for a feature that is not reliable and of little value.

@TylerLeonhardt
Copy link
Member

ping PMs @asavaritayal @joeyaiello

@TylerLeonhardt TylerLeonhardt added the bug Something isn't working label Oct 25, 2018
@TylerLeonhardt
Copy link
Member

I just saw that we got some chatter about classes support. If we keep entryPoint, we can leverage it with classes no? That seems like a reasonable use case?

@joeyaiello joeyaiello added this to the Unknown milestone Nov 28, 2018
@joeyaiello
Copy link

I'm not sure we care about entrypoint right now, but I'll spin up an issue to discuss it, and then we can close this out if we determine it's useless over there.

@joeyaiello joeyaiello changed the title "Import-Module -name script.ps1" actually loads the functions to the global scope Disable .ps1 support in entryPoint Dec 6, 2018
@joeyaiello joeyaiello modified the milestones: Unknown, Preview Dec 6, 2018
@daxian-dbw
Copy link
Contributor Author

Closed via #116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants