Revamp and move to TypeScript

[ronjouch]

Breaking changes

• Require Node >= 8.10.0 and npm 5.6.0
• Move to Electron 8.1.1.
• That's it. Lots of care went into breaking CLI & programmatic behavior
  as little as possible. Please report regressions.
• Known issue: build may fail behind a proxy. Get in touch if you use one:
  [8.0 regression] Unable to download Electron if behind a proxy #907 (comment)

Changes summary

Nativefier didn't get much love recently, to the point that it's
becoming hard to run on recent Node, due to old dependencies.
Also, some past practices now seem weird, as better expressible
by modern JS/TS, discouraging contributions including mine.

Addressing this, and one thing leading to another, came a
bigger-than-expected revamp, aiming at making Nativefier more
lean, stable, future-proof, user-friendly and dev-friendly,
while not changing the CLI/programmatic interfaces. Highlights:

• Require Node>=8, as imposed by many of our dependencies. Node 8
  is twice LTS, and easily available even in conservative Linux distros.
  No reason not to demand it.
• Default to Electron 8.
• Bump all dependencies to latest version, including electron-packager.
• Move to TS. TS is great. As of today, I see no reason not to use it,
  and fight interface bugs at runtime rather than at compile time.
  With that, get rid of everything Babel/Webpack.
• Move away from Gulp. Gulp's selling point is perf via streaming,
  but for small builds like Nativefier, npm tasks are plenty good
  and less dependency bloat. Gulp was the driver for this PR: broken
  on Node 12, and I didn't feel like just upgrading and keeping it.
• Add tons of verbose logs everywhere it makes sense, to have a
  fine & clear trace of the program flow. This will be helpful to
  debug user-reported issues, and already helped me fix a few bugs.
  • With better simple logging, get rid of the quirky and buggy
    progress bar based on package progress. Nice logging (minimal
    by default, the verbose logging mentioned above is only used
    when passing --verbose) is better and one less dependency.
• Dump async package, a relic from old callback-hell early Node.
  Also dump a few other micro-packages unnecessary now.
• A first pass of code cleanup thanks to modern JS/TS features:
  fixes, simplifications, jsdoc type annotations to types, etc.
• Remove GitHub integrations Hound & CodeClimate, which are more
  exotic than good'ol'linters, and whose signal-to-noise ratio is too low.
• Quality: Add tests and add Windows + macOS CI builds.
  Also, add a manual test script, helping to quickly verify the
  hard-to-programatically-test stuff before releases, and limit regressions.
• Fix a very small number of existing bugs. The goal of this PR was
  not to fix bugs, but to get Nativefier in better shape to do so.
  Bugfixes will come later. Still, these got addressed:
  • Add common Alt+Left/Right for previous/next navigation.
  • Improve Zoom with Ctrl+Mousewheel doesn't work #379: fix zoom with Ctrl + numpad +/-
  • Fix pinch-to-zoom (see Zoom with Ctrl+Mousewheel doesn't work #379 (comment) )

Todos left for later

• Review Electron docs / Security for mistakes, including [Security] Disable Node / Electron Integration #916 & [Security] Use chromium sandbox #756 .
• Existing bugs. The goal of this PR was not to fix bugs, but to get Nativefier in better shape to do so. Bugfixes will come later.
• Tsconfigs: make tsc strict(er), improve typing and remove remaining @ts-ignores
• GitHub: Enable dependabot
• GitHub/Travis: Setup automatic pre-release tag & build (e.g. x.y.z+1-pre6) to npm beta channel on merge

[ronjouch]

Right now this cleanup is incomplete (and ESLint frowns), but compiles, and recently started to Work For Me™. EDIT March 7: this PR is ready for review and testing, and will hopefully be merged soon.

My own use cases are very basic though, only covering a small subset of CLI flags, and only on Linux. So, I'm asking the community for help to manually test other workflows / platforms / flags and report bugs. I will fix them and will do my best to add unit & integration tests.

@jiahaog, overall feedback welcome. I tried to make this revamp as uncontroversial as possible: keeping the CLI unchanged, not adding any new feature, using proven tech, getting rid of the obsolete, and striving to make things simpler. If there's any major disagreement with what I'm doing here, please shout as soon as possible 🙂.

@remusao @saulocastelo @Aidoboy @mxdpeep @dlight @core-code @akkhilaysh @vinifmor @dipenpatel235 @3ndlos @freeze455 @voltechs @partynikko @jmalarcon @dukejones @bacongravy @codekandis (and passersby), hi! I'm pinging you because you recently showed interest in Nativefier by participating in issues discussions. I need your help to test this work-in-progress branch.

1. If you're not interested in testing or lack time, please say so by downvoting this comment with a 👎, and unsubscribe from the PR (button on the right sidebar). I'll exclude you from future requests, and sorry for the noise 🙏.
2. If you do have some time to help, thanks! Please:
   • Check out this branch: git clone git@github.com:jiahaog/nativefier.git && cd nativefier && git checkout ts
   • Build nativefier: npm install && npm run clean && npm run build && npm link
   • Test your crazy workflows / flags / platforms!
   • Report issues, prefixing them with the [ts] prefix, filling maximal debug information and include Nativefier's --verbose output. As much as possible, please only file regressions (so, validate that stable Nativefier 7.7.1 does not have the bug you're filing). One thing at a time: I want to get this merged, not try to boil the ocean by attempting to by the way fix all existing known issues 🙂.

ETA is I-don't-know, but the items I want to address before a release are listed at README / TS: Minimum to release beta. Thanks for your help!!! Let's do this!

[jiahaog]

@ronjouch This is big... and awesome! I don't know if this is ready for review, but I just did a first pass through the changes and only have some minor comments — I think you have already covered most of the TODO items in the readme.

I'll try to find some time this weekend to test the changes on a mac, but everything looks good!

[ronjouch]

@jiahaog @remusao I have a workaround for the app npm install trouble. First, let's recapitulate the situation and define things:

1. Nativefier consists in a CLI and an Electron app template, each with its own dependencies.
2. With this PR comes a new need to npm install the app's dependencies at nativefier install time, because the app template is no longer published as a webpack bundle containing everything including deps. I'm currently doing the npm install within a postinstall script, but realized this causes npm issue #757 - npm install -g of package itself calling npm in postinstall script causes constant 60s timeout.
3. Solutions considered so far considered were to:
   1. Split up the app template into its own npm package, and make the CLI depend on it.
      I dislike this approach because 1. the app template is useless on its own without the CLI, and 2. it complicates build & publishing.
   2. Come back to Nativefier <= 7.7.1's approach: distributing the app as one big self-contained webpacked main.js file, built at npm pack/publish time and no longer needing a cd app && npm install on the user's machine.
      I dislike this approach, because it undoes the gains in end-user app code hackability brought by the app being laid bare in resources/app in nice clean JS. I'm a fan of hackable, obvious technology. It saddens me to re-introduce a piece of tech that serves a need but hinders users rights.
   3. EDIT A third option, suggested by @remusao, would be to not npm install the app at Nativefier install time, and instead do it at the time of running Nativefier.
      I dislike this approach also, because it's inefficient, leading to repeated time loss on each run, which will annoy users and even more developers.

As I was, reluctantly, going back to webpack today, I thought: when npm is installing nativefier, why use npm to cd app && npm install? Could we give another tool the responsibility to do the install, and avoid npm tripping up on locks?

Turns out, we can! Plugging in yarn do the install of app dependencies works just fine. Remarks:

1. I felt dirty doing this, but then came to just thinking "why not?". I'll stay out of arguing whether or not the use case of running npm within npm is reasonable or should be supported by npm. What I say is that Nativefier isn't your regular npm library, it has specific needs, and I argued why approaches 3.{i, ii, iii} above don't satisfy me. So, we have an unaddressed specific need. Well then, let's use a specific tool to address it!
2. I initially added yarn to the deps, but felt it was wrong, since we're just using it for build. Then I remembered npm >= 5.2.0 (which we already require) provides npx for this exact need. It comes with caching too, so it's efficient across runs. Undoing this and coming back to yarn as dep because, as pointed out by Jia, we want to control the (major) version of yarn used
3. "What if the user installs nativefier through Yarn, will running Yarn inside Yarn deadlock too?" No, I tested it, and it doesn't; good job facebook.
4. This means a slight cache inefficiency, as yarn will use its own cache (~/.cache/yarn) separate from npm's (~/.npm). I'm not bothered too much about that, as Nativefier users expect to download hundreds of megabytes of Electron anyway; a few KB/MB of missed caching seems like a non-issue.

I just pushed 34ecd0c . Please try it and offer your feedback.

[remusao]

I am not very familiar with the workflow of nativefier so maybe this is dumb but, would it make sense to not install the dependencies of app/ as a postinstall hook. It seems to me that they are only needed once you package a new app by invoking the cli, so what about the following workflow instead:

0. Install nativefier globally (installs only dependencies of the CLI)
1. Invoke nativefier cli to create a new standalone electron app.
2. CLI copies content of app/ in a temporary directory unique to the input URL.
3. CLI fills the template with information.
4. CLI calls npm install in this directory.
5. CLI packages the directory into an Electron app.
6. Directory can be clean-up once the Electron app is created.

In terms of flow, it seems that calling npm install in app/ could be done by the CLI whenever a new Electron app is created, instead of when installing nativefier itself. What do you think?

[ronjouch]

calling npm install in app/ could be done by the CLI whenever a new Electron app is created, instead of when installing nativefier itself

@remusao true, it's a third option, thanks. It's not dumb.

• Pros: simple, should work.
• Cons: N more seconds of install at each nativefier call. Sounds like a minor annoyance for users, and a bigger annoyance for devs while iterating fast on code changes and testing by running nativefier lots of time in a short time.

So, for now, still preferring the yarn workaround to that.

[remusao]

calling npm install in app/ could be done by the CLI whenever a new Electron app is created, instead of when installing nativefier itself

@remusao true, it's a third option, thanks.

• Pros: simple, should work.
• Cons: N more seconds of install per nativefier call. Sounds like a minor annoyance for users, and a bigger annoyance for devs while iterating fast on code changes and testing by running nativefier lots of time in a short time.

@ronjouch Good point about the overhead, I just checked locally to see how much time it takes for the npm install command (with my pretty bad internet bandwidth):

1. Initial npm install: ~4 seconds
2. Subsequent npm install: 1-2 seconds

Interestingly, the same workflow with Yarn seems much faster, especially subsequent calls to yarn install which take ~0.300 seconds. Maybe nativefier could use yarn internally instead of npm to handle this installation step and make sure that it does not impact the dev workflow too much?

[My6UoT9]

@My6UoT9 excellent news, I hadn't tested Windows yet, thanks a lot for the test 🙂! Do you confirm all the flags described work? (tray, shortcuts, icon, single instance)?

Yes all flags worked, in the meantime extended it to user browseroptions too. There is just on issue related to windows, I first wrote a batch file (*.bat), so I can repack it at anytime. But for some reason the passed json string does not work. Then I tried directly in a cmd, and that worked.
Customized for my use case, it opens the website on right side of screen just as I want it.

nativefier "https://mixcloud.com" --name "Mixcloud" --tray --global-shortcuts shortcuts-mixcloud.json --icon mixcloud2.ico  --single-instance --win32metadata "{\"ProductName\": \"Mixcloud\", \"InternalName\": \"Mixcloud\", \"FileDescription\": \"Mixcloud player\"}" --browserwindow-options "{ \"skipTaskbar\": true,  \"frame\": false, \"height\":1200, \"width\":1025,  \"x\":850, \"y\": 0, \"resizable\":false, \"movable\":false, \"webPreferences\": { \"webgl\":false} }"

[jiahaog]

@ronjouch I'm wondering if using any of the other hooks besides postinstall will allow us to workaround the issue. But if that doesn't work, using yarn is a pretty clever workaround 😂

[UndarkAido]

How would one test this with a project that depends on Nativefier via NPM? I use it with https://github.com/Aidoboy/nativefier-install

[UndarkAido]

@ronjouch Ok, cool! I'll try to poke it a bit soon. I've been pretty busy recently.

[ronjouch]

@Aidoboy the last commit called Expose buildMain wrapped in hopefully-backwards-compatible callback-style function as default export should be usable by your existing JS project importing nativefier, with no change.

It's untested, so probablymaybecertainly broken right now, but not irremediably, and fixable with your feedback. Please give it a shot and shout back! Also, all general feedback regarding this revamp very welcome; file issues with a [ts] prefix (see the last point below). Instructions to try this branch:

• Check out this branch: git clone git@github.com:jiahaog/nativefier.git && cd nativefier && git checkout ts

• Build nativefier: npm install && npm run clean && npm run build && npm link

• Test your crazy workflows / flags / platforms!

• Report issues, prefixing them with the [ts] prefix, filling maximal debug information and include Nativefier's --verbose output. As much as possible, please only file regressions (so, validate that stable Nativefier 7.7.1 does not have the bug you're filing). One thing at a time: I want to get this merged, not try to boil the ocean by attempting to by the way fix all existing known issues 🙂.

[tristangodfrey]

Boy am I glad my spidey senses kicked in when I started working on this myself last night! Are you still working on this @ronjouch? Seems like just small fixes from here on out, let me know if you need a hand.

Nice work by the way! 🎆

[ronjouch]

Boy am I glad my spidey senses kicked in when I started working on this myself last night! Are you still working on this? Seems like just small fixes from here on out, let me know if you need a hand. Nice work by the way!

@tristangodfrey thanks! Yes, this is still my current code project these days, progressing when I have time on evenings and weekends.

ETA is probably a few weeks, or months if I'm lazy. The remaining tasks before a release are listed at README / TS: Minimum to release beta. If you want to give a hand, feel free to grab one of these and submit a PR!

[voltechs]

I've been a bit of a recluse here. I came up for some air and gave this a try. Things seem to be working pretty well. I still can't seem to get --inject for custom CSS or JS working. Additionally, do you think we'll be able to "install" some browser/chrome compatible plugins? For example, I'd love to be able to get 1Password installed and working on these, and I'd love to be able to install Adblocker, lol. Thoughts?

Examples of apps I build/use with nativefier.

nativefier --name "Google Calendar" 'https://calendar.google.com'  --darwin-dark-mode-support --inject https://raw.githubusercontent.com/pyxelr/Dark_Google_Calendar/master/Style.user.css
nativefier --name "JumpCloud" --icon Nativefier/JumpCloud.png 'https://console.jumpcloud.com'  --darwin-dark-mode-support

Thanks for all the hustle and hard work, everyone!

[ronjouch]

@voltechs thanks for the feedback.

Things seem to be working pretty well.

👍, good to know. Which OS are you running, by the way?

I still can't seem to get --inject for custom CSS or JS working.

Thanks for the report, but I cannot reproduce the issue. --inject works for me, css is applied and js is run. Please provide OS/Node.js details, the full nativefier command, and --verbose logs.

EDIT oh, I see an example in your previous message: nativefier --name "Google Calendar" 'https://calendar.google.com' --darwin-dark-mode-support --inject https://raw.githubusercontent.com/pyxelr/Dark_Google_Calendar/master/Style.user.css . And true enough, nativefier chokes on the URL (it wants a local file, not a remote one), but so does 7.7.1 stable. Both currently skip the injection and log Error copying injection files Error: Error copying injection files: file not found. I just pushed a commit clarifying this, you will now see Error copying injection files. Error: File https://raw.githubusercontent.com/pyxelr/Dark_Google_Calendar/master/Style.user.css not found. Note that Nativefier expects *local* files, not URLs. Was that the sole problem, or did you notice anything else?

do you think we'll be able to "install" some browser/chrome compatible plugins? For example, I'd love to be able to get 1Password installed and working on these, and I'd love to be able to install Adblocker, lol. Thoughts?

• Chrome extensions: see Support packaging Chrome extensions #207 . TL;DR: don't count on it.
• Basic adblocking: might be feasible, I plan to look at it after this revamp lands. See Support basic ad blocking (maybe through @cliqz/adblocker-electron, or another lib, or homebrew) #177 .

[jiahaog]

Thank you so so much for taking the time to do this!! I ran some basic workflows on a mac and it works great as well. Please let me know if you need any help merging / publishing this! Also, will you be squashing the commits when you do the merge?

[jiahaog]

What does this comment mean?

[ronjouch]

What does this comment mean?

@jiahaog see a few lines below; with the addition of {Windows, macOS} platforms in Travis, CI duration lengthened, so I added a condition restricting the linter to run only on a single sub-build, to shave off 10s * 8 from the build:

# Only run linter once, for faster CI
- if [ "$TRAVIS_OS_NAME" = "linux" ] && [ "$TRAVIS_NODE_VERSION" = "13" ]; then npm run lint; fi

I just pushed a little improvement of the comment to clearer # Changing this? Remind to adjust the linter condition below causing linter to run for only one version (for faster CI)

---

Thank you so so much for taking the time to do this!! I ran some basic workflows on a mac and it works great as well.

@jiahaog welcome 🙂, and thanks for the final tests. I use lots of open-source software at home and at work, this is for me an occasion to give back in a context that I know (I do TS at work these days), and contribute a lot by cleaning & strengthening a codebase that needed it. The last few weeks I got confident that at least common use cases will be fine, as:

1. I got a fair amount of feedback in the last weeks, both in this PR and also in a few existing issues where I suggested reporters to give this branch a try because it fixes their issues. Feedback is largely positive, and a few minor remaining bugs were reported and fixed.
2. The added tests, added CI runs for all platforms, scripting of a pre-publish manual test, and final exploratory testing on Linux & Windows, are all reassuring.

There will inevitably be regressions given the amount of changes, but I'm not anticipating a tornado of bug reports. I'm subscribed to issues notifications and will keep a close eye on them after releasing, fixing regressions quickly in patch versions.

Please let me know if you need any help merging / publishing this! Also, will you be squashing the commits when you do the merge?

@jiahaog I'll be merging this as 8.0.0 today or next week, with the description of this PR as release notes. I will squash (a few squashes already happened, for rebase simplicity). I don't anticipate needing help merging & publishing (the normal release process should work), but will ping you if something I forgot about goes wrong.

Thanks for the support and reviews, cheers!

[AndriusWild]

Right now this cleanup is incomplete (and ESLint frowns), but compiles, and recently started to Work For Me™. EDIT March 7: this PR is ready for review and testing, and will hopefully be merged soon.

My own use cases are very basic though, only covering a small subset of CLI flags, and only on Linux. So, I'm asking the community for help to manually test other workflows / platforms / flags and report bugs. I will fix them and will do my best to add unit & integration tests.

@jiahaog, overall feedback welcome. I tried to make this revamp as uncontroversial as possible: keeping the CLI unchanged, not adding any new feature, using proven tech, getting rid of the obsolete, and striving to make things simpler. If there's any major disagreement with what I'm doing here, please shout as soon as possible slightly_smiling_face.

@remusao @saulocastelo @Aidoboy @mxdpeep @dlight @core-code @akkhilaysh @vinifmor @dipenpatel235 @3ndlos @freeze455 @voltechs @partynikko @jmalarcon @dukejones @bacongravy @codekandis (and passersby), hi! I'm pinging you because you recently showed interest in Nativefier by participating in issues discussions. I need your help to test this work-in-progress branch.

1. If you're not interested in testing or lack time, please say so by downvoting this comment with a -1, and unsubscribe from the PR (button on the right sidebar). I'll exclude you from future requests, and sorry for the noise pray.

2. If you _do_ have some time to help, thanks! Please:
   
   * Check out this branch: `git clone git@github.com:jiahaog/nativefier.git && cd nativefier && git checkout ts`
   * Build nativefier: `npm install && npm run clean && npm run build && npm link`
   * Test your crazy workflows / flags / platforms!
   * Report issues, prefixing them with the `[ts] ` prefix, filling maximal debug information and include Nativefier's `--verbose` output. As much as possible, please _only file regressions_ (so, validate that stable Nativefier 7.7.1 does not have the bug you're filing). One thing at a time: I want to get this merged, not try to boil the ocean by attempting to by the way fix all existing known issues slightly_smiling_face.

ETA is I-don't-know, but the items I want to address before a release are listed at README / TS: Minimum to release beta. Thanks for your help!!! Let's do this!

It looks like the repository isn't public. I keep getting this error:

git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.