Skip to content
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

Initial hyper session is not created correctly #3770

Closed
2 tasks done
stnwk opened this issue Jul 27, 2019 · 8 comments · Fixed by #3937
Closed
2 tasks done

Initial hyper session is not created correctly #3770

stnwk opened this issue Jul 27, 2019 · 8 comments · Fixed by #3937
Labels
🐛 Type: Bug Issue pertains to something wrong within Hyper
Milestone

Comments

@stnwk
Copy link

stnwk commented Jul 27, 2019

  • I am on the latest Hyper.app version
  • I have searched the issues of this repo and believe that this is not a duplicate
  • OS version and name: macOS 10.14.5
  • Hyper.app version: 3.0.2

Issue

Hi :) Thanks for your work on this!

I use https://github.com/hharnisc/hypercwd and noticed the following bug hharnisc/hypercwd#71:

The problem is:
The initial working directory is not set correctly for the very first hyper session, but people noticed that after a full reload the CWD was set correctly again. The issue on hypercwd seems a bit stale and nobody seems to know how to fix it, so I took a little dive into the code and this is what I found:

First: the bug does not seem to come from the hypercwd codebase, but rather from this codebase.

The problem seems to be located in the optimistically/eagerly creating an initial session here: https://github.com/zeit/hyper/blob/c13c8a5c52cbc8e02098244193a4e715fffcd902/app/ui/window.js#L140
To be more exact in createInitialSession():
https://github.com/zeit/hyper/blob/c13c8a5c52cbc8e02098244193a4e715fffcd902/app/ui/window.js#L124

Here createSession() is optimistically called without passing any parameter, although the function is defined as createSession(extraOptions = {}). Those extraOptions are used to set the CWD (among other things correctly), see: https://github.com/zeit/hyper/blob/c13c8a5c52cbc8e02098244193a4e715fffcd902/app/ui/window.js#L111

    function createSession(extraOptions = {}) {
      const uid = uuid.v4();

      const defaultOptions = Object.assign(
        {
          rows: 40,
          cols: 100,
          cwd: process.argv[1] && isAbsolute(process.argv[1]) ? process.argv[1] : homeDirectory,
          splitDirection: undefined,
          shell: cfg.shell,
          shellArgs: cfg.shellArgs && Array.from(cfg.shellArgs)
        },
        extraOptions, // <-- this is the important line
        {uid}
      );
      const options = decorateSessionOptions(defaultOptions);
      const DecoratedSession = decorateSessionClass(Session);
      const session = new DecoratedSession(options);
      sessions.set(uid, session);
      return {session, options};
    }

This optimistically creating an initial session without passing the correct extraOptions is the underlying problem for the mentioned issue. The CWD is set incorrectly, because it is not set at all. After a full reload however it is then overwritten correctly.

I am unsure about the reasoning for this optimistic session creating and whether it is a technical necessity or a performance optimization.

I tried removing https://github.com/zeit/hyper/blob/canary/app/ui/window.js#L140 this line and everything seemed to still work and the bug was fixed.

I hope this analysis provides enough insight for you to come up with a decision on how to proceed quickly as the issue really is quite annoying.

Thanks for your help, have a nice day :)

@stnwk
Copy link
Author

stnwk commented Aug 13, 2019

@chabou Friendly ping :) Can you please take a look at this? It's a really annoying issue :/ Thanks!

@libiseller
Copy link

Any progress?
It's seems like an easy fix and it's really annoying.
Every extension relying on extraOptions is broken... @chabou

@ahmedx2
Copy link

ahmedx2 commented Sep 10, 2019

I would like to bump this issue as well. I also find this issue to be annoying when using hypercwd and other Hyper plugins. It seems like a fix should be simple enough since @stnwk has already done such stellar investigative work.

@stnwk
Copy link
Author

stnwk commented Sep 10, 2019

@ahmedx2

It seems like a fix should be simple enough since @stnwk has already done such stellar investigative work.

Thanks ☺️

@Stanzilla Stanzilla added the 🐛 Type: Bug Issue pertains to something wrong within Hyper label Oct 12, 2019
@Stanzilla Stanzilla added this to the 3.1.0 milestone Oct 12, 2019
@Stanzilla
Copy link
Collaborator

Any of you want to create PR for this?

@stnwk
Copy link
Author

stnwk commented Oct 12, 2019

Happy to do so.

Will create a PR with the suggested solution I outlined in my initial analysis of this problem :)

@stnwk
Copy link
Author

stnwk commented Oct 29, 2019

Update: Sorry, I thought I'd have the resources to do this, but unfortunately currently I don't.

Happy if someone else would like to come up with a PR.
A possible solution was already described in the original post.

@ahmedx2
Copy link

ahmedx2 commented Oct 30, 2019

@Stanzilla @stnwk Done: #3937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Type: Bug Issue pertains to something wrong within Hyper
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants