Fix: notifications (fix #88, fix #956), processEnvs, using as git

[exander77]

1. Fix (broken since 2016): Notifications broken by lambda constructor
2. Fix: --processEnvs broken by additional processEnvs object, the result was:
   processEnvs: {processEnvs: {...}} which caused the conversion of the inner object into string [object Object], no nesting allowed there probably. Compatibility introduced.
3. Fix: package.json missing prepare (or even prepublish), which breaks using as git dependency.

[ronjouch]

@exander77 hi, thanks for the contribution. Please elaborate: what was broken?

[exander77]

A constructor cannot be a lambda function, so this completely broke new Notification as Notification was not a constructor, just a regular function. Notifications were probably broken since then.

[exander77]

I have traced it back to 2016, to see what happened:

-    var OldNotify = window.Notification;
-    var newNotify = function(title, opt) {
+    const OldNotify = window.Notification;
+    const newNotify = (title, opt) => {

[ronjouch]

A constructor cannot be a lambda function, so this completely broke new Notification as Notification was not a constructor, just a regular function. Notifications were probably broken since then.

@exander77 okay, so the symptom was that notifications were broken, and you tested that re-building with this change fixes them. Correct?

Good to know, I didn't know about that. Reading more about it, e.g. in SO / Why cannot use lambda to define prototype function.

[exander77]

@ronjouch Yes, I was debugging why are they not working. Also, the code was previously there, so I am basically reverting.

Btw, lambda is not just fancy name how to write functions. It is a concept derived from Lambda Calculus, where you create ad hoc anonymous functions. Constructors are named and they are bound to instance because they have access to this, lambda, on the other hand, is accessing this of the parent closure.

Also in the future, those lines should be completely rewritten to TypeScript. I may do it. A side question, why is this code in preload and not main, what is the logic there?

Btw, please merge it soon, as this fixes things for a lot of apps and users. I may be making more pull requests, I just found out processEnvs seems to be broken as well. I am getting: [object Object] so, probably a conversion broken somewhere. Do you guys accept feature improvements as well? I would like not to have permanent for of this project, better to push fixes etc. here.

[exander77]

Updated with more fixes. I am using these two days and based on my experience so far, you guys tend to rewrite code and break it for no reason and probably not even test.

I think this is a good project, but this type of development hampers the usefulness.

[ronjouch]

@exander77

Yes, I was debugging why are they not working. Also, the code was previously there, so I am basically reverting.

Awesome.

Also in the future, those lines should be completely rewritten to TypeScript. I may do it. A side question, why is this code in preload and not main, what is the logic there?

No idea.

Btw, please merge it soon, as this fixes things for a lot of apps and users. I may be making more pull requests, I just found out processEnvs seems to be broken as well. I am getting: [object Object] so, probably a conversion broken somewhere.

Yup, I'll merge & release this as soon as I can review and test it.

Do you guys accept feature improvements as well? I would like not to have permanent for of this project, better to push fixes etc. here.

Fixes and improvements welcome; see recent announcement asking for help.

I am using it these two days and based on my experience so far, you guys tend to rewrite code and break it for no reason and probably not even test. I think this is a good project, but this type of development hampers the usefulness.

Your improvements are welcome, but your focus on blaming people, lecturing attitude, and idealistic context-less comments full of unmet assumptions, are not welcome.

1. First, there is no "you guys", there's just me, and the creator left a long time ago.
2. I'm no longer using Nativefier at all, except for a single extremely simple and occasional use case.
3. I'm not interested in maintaining it, but I do it because it has lots of users and I myself depend on a lot of open-source, so doing the maintenance is an act of giving back.
4. You are wrong when you say I rewrote Nativefier "for no reason". Nativefier, before the 8.x TS rewrite, was extremely broken, to the point of being impossible to build with recent Node. It was also full of weird early Node patterns, making it difficult to approach for newcomers. Being no longer a user, but seeing tons of unaddressable issue reports, and wanting to provide something stable and future-proof for users and contributors, I took it upon myself to modernize it, doing my best to make it "lean, stable, future-proof, user-friendly and dev-friendly, while not changing the CLI/programmatic interfaces" (see PR #898 - Revamp and move to TypeScript). I knew Nativefier was badly tested, so I 1. added tests and 2. asked for testing help from the community. But apart from adding a heroic amount of tests, there was no way I could succeed at introducing zero regressions. I already fixed a few (see changelogs for 8.0.{3,4,5,6,7}), and you're fixing one, that's good and that's how progress happens.

I hope you're able to reconsider what you said, can appreciate what I wrote above, and can present excuses. Your comment made me angry on a Sunday morning and that's not what I want from a contributor.

[exander77]

I have seen multiple emails in the commit history, hence "guys". Breaking of Notifications is from 2016, not related to Revamp. Don't take it personally, it is not an attack or anything like that, it is more of frustration from debugging problems. I complain a lot about things... but do not take me so seriously! It is not meant in a bad way. I have seen the history and I know a Revamp was a large piece of good work! I went through a lot of old commits to see how the project evolved and where the breakage come from.

You are right, that I am not focusing on positive things much. This is because it seems obvious I like the project and instead of abandoning it after problems, I will try to fix problems when I find them.

What I am looking now at and I would appreciate any input is:

1. What happens to the arguments passed to the actual program? In process.argv are some arguments passed to electron I think, which were not actually passed on the command line.
2. How to access tray icon (I would like to change it programmatically during execution)? Seems it is not accessible from injecting at all.

I really think this is a good project.

And I am sorry for making you angry, don't be. Just ignore me, If I am complaining too much...

[exander77]

I think, the notification affects these two:
#88
#956

I can make a PR with moving notification code from preload to main and adding flag for notifications disabling:
#955
Maybe even set it to true, to not break anybody's workflow.

[ronjouch]

@exander77 thanks for the acknowledgements an excuses 🙂. To close on that, take this little clash as a reminder that there are sometimes good reasons for the state you find code (or society?) in. Then, if/when frustration about the current state starts boiling up, remember to make less assumptions and ask questions before criticizing. I don't want to ignore you, I'm grateful for your contributions.

---

Now, back to the problem:

What happens to the arguments passed to the actual program? In process.argv are some arguments passed to electron I think, which were not actually passed on the command line.

Hmmmmm arguments passed to which actual program? Are you talking about build-time args? They're processed, massaged in several ways, and persisted to a nativefier.json file living in the electron app folder. See the code flow from cli.ts.

How to access tray icon (I would like to change it programmatically during execution)? Seems it is not accessible from injecting at all.

No sé. Search in the Electron docs, maybe in library code, and make a few Google searches.

I think the notification affects these two: #88 #956

👍! Please append (Fix #88, Fix #956) to your issue title, that will close the issues automatically on PR merge. Also, a concise explanation of the problem in the PR description (which will become the commit body) is welcome. Usual pointer to my favorite short talk on the subject: Greg Ward - Documenting history, or how to write great commit messages: say why, not what.

I can make a PR with moving notification code from preload to main and adding flag for notifications disabling: #955 . Maybe even set it to true, to not break anybody's workflow.

Can we recap together the current/expected behavior?

• Notifications must have been working before 2016.
• But since then, they must have been broken.

So, right now, users might assume they're "disabled by default" (except they're just broken 😄). So your code could "keep them disabled" by default, with an --enable-notifications flag to enable them. That seems reasonable to me, yes.

Thanks!

[exander77]

Thank you for understanding!

I am talking about when you call the actual binary built by nativefier with some arguments:
./Test-linux-x64/Test --alfa --beta=gama

I will go through the bug reports and tag all appropriate to the PR.

Yes, exactly, the sudden appearance of notifications after so many years may negatively surprise users. :D

[exander77]

I am also looking into MacOS specific --counter option. I suggest an alternative for Linux etc. by modifying tray icon. See attached.

Looks hideous zoomed like that, but you probably get the picture. Behaviour would be the same.

[ronjouch]

Suggested change

	"prepare": "cd ./app && npm install && cd .. && npm run clean && tsc --build . app && npm run build-app && npm run build-app-static",
	"prepare": "npm run dev-up && npm run build",

[exander77]

I will try, but npm install is actually run automatically when you install it as a dependency. I am not sure if it is wise to interfere with it as npm run dev-up calls it as well, but npm run build should be safe I think.

[exander77]

I changed it to cd ./app && npm install && cd .. && npm run build. When I go full npm, it is going into some kind of cycle. I think, when you call npm install, it will call prepare, and then prepare will call install etc.

[ronjouch]

I'd rather keep the separate tasks we have, then, your prepare task is confusing, as it only npm installs the app. The current tasks are find and documented in https://github.com/jiahaog/nativefier/blob/master/docs/development.md :

• dev-up for initial npm installs of cli and app
• then build to build

[ronjouch]

Oh okay, I'm reading the updated PR description where you say Fix: package.json completely missings prepare (or even prepublish) which breaks using as git dependency. Alright.

[exander77]

@ronjouch This is not my task at all. This a task required by npm to able to use dependency from git: https://docs.npmjs.com/misc/scripts

If you specify main and bin in your package.json, you need to have those files there after npm itself calls these on the dependency:

npm install
npm run prepare

This is also why I cannot put npm install in the prepare, as npm which is also calling it will run into infinite cycle.

[ronjouch]

@exander77 got it, see my comment above:

Oh okay, I'm reading the updated PR description where you say Fix: package.json completely missings prepare (or even prepublish) which breaks using as git dependency. Alright.

[exander77]

Yes, I am updating it with new info. As I understand it, when you add a dependency from git, npm calls this on it:

npm install
npm run prepare

And then checks if main and all bin are there and fails completely if they are not there. :(

[ronjouch]

I am also looking into MacOS specific --counter option. I suggest an alternative for Linux etc. by modifying tray icon. See attached.

Looks hideous zoomed like that, but you probably get the picture. Behaviour would be the same.

LGTM. If you can, a separate PR will be welcome, I like commits documenting and working on one thing, it makes for easier history digging & bisection.

@exander77 I can already merge the current PR. Is it ready, or do you have other things close by you want to add?

[exander77]

@ronjouch Yes, merge it, please. This should be a small PR with fixes.

[pgrenaud]

I just discovered this project one hour ago, built an app for Google Calendar, noticed that the notifications are not working then found this. I'm glad that this issue got addressed and getting fix soon™️! I cannot wait for building more apps! Keep up the good work!

(I'm commenting here to get notified once this is merged.)

[exander77]

@pgrenaud When it is merged, give us a notice if everything is going ok for you!

Also, I think you can check out a pull request locally and test it if you want to:
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally
https://github.com/genome/docs/wiki/How-do-I-`git-checkout`-a-pull-request%3F

[ronjouch]

Thanks @exander77.

Tell me if you have new stuff brewing and ready soon, I'll either release this soon or wait for your new stuff.

[exander77]

@ronjouch I will add PR with notification switch soon, hopefully by the end of this week.

[schloram]

Hi everybody,
thanks to @exander77 to approach this bug. But one thing to clarify:

• Notifications must have been working before 2016.
• But since then, they must have been broken.

That is not correct. Notifications worked with nativefier 7. I created the same app with the exact same settings with v7 back in February and another one after an update to v8 just now. The first has notifications working the other one doesn't.

Nevertheless, it is broken now and good thing @exander77 fixed it. :)

[exander77]

@schloram Version 7 was released in 2016. The commit which broke it is dated 2016. So you were using a version which was not affected yet.

[schloram]

I was actually using Nativefier v7.7.1 released this on 24 Jan. 🤔

[pgrenaud]

Hey @exander77! I never got back to you. It's been more than 2 weeks that I've been using the 2 apps I build with your fixes and they are working great! No issue so far. Thanks again.

[adshrc]

Is it working for you guys? For me it's still the same behaviour 😢

[needo37]

Still no notifications here on Linux using KDE.

[schloram]

Same here. But I figured that for some applications notifications work (e.g Whatsapp) and for others it doesn't (e.g 3CX).

Could this be because of the different implementation of browser notifications of these apps?

@needo37 @adshrc Which websites don't show applications for you? And do Whatsapp notifications work properly for you?

[adshrc]

For me it's not working with Asana. But Google Chat works!

I'm doing a little research - it definitely depends on how the app is handling Notifications. Asana replaces the native window.Notification Object with its own logic. Google Chat does not.

[noizo]

MacOS notifications not working with nativefier 11.0.2 and outlook365 portal.

nativefier --icon "~/Desktop/Outlook.icns" --darwin-dark-mode-support --single-instance --background-color '#2e2e2e' --internal-urls ".*(office|microsoftonline|office365|sharepoint|microsoft|onenote)\.(com).*" --counter  --bounce --disable-context-menu --disable-dev-tools --zoom 0.9 --name "Outlook-Web" --browserwindow-options '{ "fullscreenable": "true", "simpleFullscreen": "false" }' "https://outlook.office365.com/" ~/Applications

My script to create Outlook app, but no native OS notifications so far.