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

Add migration path from Hyper2 config location to Hyper3 #3610

Merged
merged 6 commits into from
May 8, 2019

Conversation

juancampa
Copy link
Contributor

This PR adds a migration path from Hyper2 config file locations to Hyper3 file locations.

The idea is to use the old file by moving it to the new location. While creating back ups for any existing files.

The old file is also renamed so that this happens only on the first run.

  • Test on Linux
  • Test on MacOS
  • Test on Windows

@juancampa juancampa requested review from Stanzilla and chabou May 6, 2019 21:52
@Stanzilla
Copy link
Collaborator

image

@juancampa
Copy link
Contributor Author

juancampa commented May 6, 2019

@Stanzilla fixed. Weird that I have that installed as a indirect dependency but you don't. Anyway that's a terrible module that I'm hoping goes away when we update dependencies:

$ yarn why fs-extra-p
yarn why v1.15.2
[1/4] Why do we have the module "fs-extra-p"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "fs-extra-p@7.0.0"
info Reasons this module exists
   - "electron-builder-squirrel-windows" depends on it
   - Hoisted from "electron-builder-squirrel-windows#fs-extra-p"
   - Hoisted from "electron-builder#fs-extra-p"
   - Hoisted from "electron-builder#app-builder-lib#fs-extra-p"
   - Hoisted from "electron-builder#dmg-builder#fs-extra-p"
   - Hoisted from "electron-builder#builder-util#fs-extra-p"
   - Hoisted from "electron-builder#read-config-file#fs-extra-p"
   - Hoisted from "electron-builder#builder-util-runtime#fs-extra-p"
   - Hoisted from "electron-builder#builder-util#temp-file#fs-extra-p"
   - Hoisted from "electron-builder#app-builder-lib#electron-publish#fs-extra-p"
info Disk size without dependencies: "328KB"
info Disk size with unique dependencies: "1.46MB"
info Disk size with transitive dependencies: "2.25MB"
info Number of shared dependencies: 6
Done in 0.97s.

@Stanzilla
Copy link
Collaborator

Stanzilla commented May 6, 2019

So from a technical standpoint, the migration works, but it would be nice to get some message out to the user as to what happened.

Could just be a simple alert ala Hey our settings location changed, you can now find it at <new path> instead of <old path>, we already migrated your old configuration for you!

@juancampa
Copy link
Contributor Author

Could just be a simple alert ala Hey our settings location changed, you can now find it at <new path> instead of <old path>, we already migrated your old configuration for you!

@Stanzilla, Added this but also rewrote it so that it handles plugin migration in the same way

@Stanzilla
Copy link
Collaborator

Stanzilla commented May 7, 2019

@juancampa Three things here:

a) the migration did not work here, the node_modules folder was empty and .hyper.js was not copied/renamed to .backup. Could be that step died because it took too long, node_modules are huge.
b) I don't think we need to migrate the plugins at all, since they will get reinstalled from package.json on first launch anyway
c) the only additional part we should migrate is custom made plugins that are in the hyper_plugins/local folder since those can't be installed via package.json

@juancampa
Copy link
Contributor Author

Thanks! I'll fix that. I haven't got time to thoroughly test this

@ppot
Copy link
Contributor

ppot commented May 7, 2019

Frankly: Should not update install code. but rather do it with scripting. Such as a prompt: file found. want to move file to new destinations or not?

@ppot
Copy link
Contributor

ppot commented May 7, 2019

We should totally rename hyper.js for hyper.conf js script will still interpret is as javascript imo.

@juancampa
Copy link
Contributor Author

juancampa commented May 7, 2019

Frankly: Should not update install code. but rather do it with scripting. Such as a prompt: file found. want to move file to new destinations or not?

Thanks @ppot, I actually considered that option, but I don't see any reason for users not wanting their files migrated, so why not do it automatically? There are also other complications that I don't think are worth handling right now: dialog would keep popping up forever until they migrate? Or just once? what if they dismiss it by accident? AFAIK we don't have a convenient way to add an arbitrary action to a popup, except for a URL link.

I definitely agree about the name being weird! I don't want to introduce more breaking changes atm though

@ppot
Copy link
Contributor

ppot commented May 7, 2019

@juancampa Why pop-up? I was implying of a shell script. With a question in a shell pty session and response so a single flag on update or install after this. nada.

@juancampa
Copy link
Contributor Author

@ppot I see what you're saying, I like that approach because it can show any errors to the user. Main issue is that we'd need to write a portable bash script and a windows version. And some of my questions above are still valid (when do we stop showing it? where do we store this state?)

@juancampa
Copy link
Contributor Author

OTOH this works and we need to get this out ASAP. It's already too late unfortunately :(

@Stanzilla
Copy link
Collaborator

@juancampa what about my point c)?

@ppot
Copy link
Contributor

ppot commented May 8, 2019

@juancampa @Stanzilla Here why my shell script since we can export the whole dir on windows and on linux. We can execute one powershell script on WSL and a bash script on OS X/ linux

@Stanzilla
Copy link
Collaborator

Small problem with the last commit, you copy the local dir but also the local.backup dir

@juancampa
Copy link
Contributor Author

juancampa commented May 8, 2019

@Stanzilla, can't repro, are you testing the latest latest? I changed the order so that I first copy and then create the backup folder 🤔, so local.backup shouldn't be there when I copy.

@Stanzilla
Copy link
Collaborator

I tested 41e4e12 let me try again

@juancampa
Copy link
Contributor Author

Oh I think what you're seeing is the backup of the new local/ in the destination path (which is probably empty)

@Stanzilla
Copy link
Collaborator

Stanzilla commented May 8, 2019

Yeah, it is in the destination dir but it is not empty. Fun fact, this only happens when running the installer, not when starting Hyper normally.

image

Otherwise seems to be good apart from one thing. If we don't copy package.json as well, Hyper won't install the non-local addons on first run and needs a manual Plugins -> Update.

@juancampa
Copy link
Contributor Author

juancampa commented May 8, 2019

@Stanzilla so many edge cases! Ok adding package.json. Thanks a lot for the support and guidance, as you can see I'm not familiarized at all with this part of Hyper

@juancampa
Copy link
Contributor Author

Oh actually, I'm not going to migrate it. The notification says to restart hyper, that should also do it, right?

@Stanzilla
Copy link
Collaborator

Hrm not the best of UX but should be okay.

@pcnate
Copy link

pcnate commented May 8, 2019

Does this check whether the new file has already been modified by the user? many of us have already done this.

@juancampa
Copy link
Contributor Author

@pcnate It doesn't but it backs it up for that case

@pcnate
Copy link

pcnate commented May 8, 2019

@juancampa good enough. looks like needs testing. is there a short howto for how to test PRs?

@juancampa
Copy link
Contributor Author

@pcnate, what I've been doing is testing different scenarios, installling from scratch with old files, without old files, etc.

@Stanzilla would you say that it works correctly on Windows?

@Stanzilla
Copy link
Collaborator

@juancampa sadly not, a restart does not trigger the installation of plugins, a manual trigger is still required, copying package.json also did not help.

@Stanzilla
Copy link
Collaborator

That might just be a hyper bug in general, maybe it should update plugins right after startup?

@juancampa
Copy link
Contributor Author

yeah, it works on Linux so I'm surprised about that one

@juancampa
Copy link
Contributor Author

Merging this and running more tests on windows before publishing a canary

@juancampa juancampa merged commit a5f86d5 into vercel:canary May 8, 2019
@voronoipotato
Copy link

not sure if this applies but I saw some .hyper.js hardcoded floating around.

#3500

:) hope that helps and if not, sorry for bothering!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants