Fix injecting multiple css/js files (fix #458) #1162

TheCleric · 2021-04-27T16:24:18Z

Fixes #458

src/build/prepareElectronApp.ts

When I added this documentation originally, I guess I placed it in the wrong location.

…tfalls Follow-up of nativefier#1159 (comment)

…ivefier#405) (PR nativefier#1154) Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

…e icon (fix nativefier#942, fix nativefier#668) (nativefier#1156) This fixes: 1. A startup crash on macOS when using the `--tray` option; see nativefier#527. ![image](https://user-images.githubusercontent.com/22625791/115987741-99544600-a5b6-11eb-866a-dadb5640eecb.png) 2. Invisible tray icon on macOS; see nativefier#942 and nativefier#668. ![image](https://user-images.githubusercontent.com/22625791/115988276-24364000-a5b9-11eb-80c3-561a8a646754.png) Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

…O Max & Udemy work (fix nativefier#1147)

…er#1163) Should alleviate part of the issue in nativefier#1151

…tivefier#1138) This adds a `--upgrade` option to upgrade-in-place an old app, re-using its options it can. Should help fix nativefier#1131 Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

… docs (fix nativefier#1153) (PR nativefier#1164) As documented in nativefier#1153, for Widevine support to work properly, we need to listen for the Widevine ready event, and as well for certain sites the app must be signed. This PR adds the events, and as well adds better documentation on the signing limitation. This may also help resolve nativefier#1147

TheCleric · 2021-04-29T18:29:54Z

@ronjouch take a look and make sure I addressed your concerns please.

ronjouch

A few questions, and nits that I'll apply directly. I merged master, too.

If the questions are clear / unproblematic to you, 👍 to merge.

docs/api.md

src/build/prepareElectronApp.ts

ronjouch · 2021-04-30T10:08:58Z

src/build/prepareElectronApp.ts

-  const postFixHash = hash.digest('hex').substring(0, 6);
+function normalizeAppName(appName: string): string {
+  // use a simple random string to prevent collision
+  const postFixHash = generateRandomSuffix();


By the way, I see you commented on an issue about this hash/suffix. This is related to #888 . Is the reason for this hash clear to you, and do you think we should keep doing it?

It is clear to me, and I do think we should keep doing it. I do not see the harm in it as long as we reference the app name correctly (i.e., user provided/inferred) where appropriate instead of this internal one.

ronjouch · 2021-04-30T10:09:42Z

src/helpers/helpers.test.ts

@@ -34,3 +34,25 @@ describe('isArgFormatInvalid', () => {
    expect(isArgFormatInvalid('--test-run-with-many-dashes')).toBe(false);
  });
 });
+
+describe('generateRandomSuffix', () => {


) This is a follow-up of #1162 (comment) PR #1162 introduced a new `generateRandomSuffix` helper function, used it for its needs (avoiding collisions of injected js/css). But it also replaced existing appname normalizing logic with it, introducing randomness in a place that used to be deterministic. As a result, starting with dd6e15f / v43.1.0, re-creating an app would cause the app to use a different appName, thus a different appData folder, thus losing user data including cookies. This PR leaves the `--inject` fixes of #1176, but reverts the appName logic to the pre-#1176 code.

) (PR #1179) This is a follow-up of #1162 (comment) PR #1162 introduced a new `generateRandomSuffix` helper function, used it for its needs (avoiding collisions of injected js/css). But it also replaced existing appname normalizing logic with it, introducing randomness in a place that used to be deterministic. As a result, starting with dd6e15f / v43.1.0, re-creating an app would cause the app to use a different appName, thus a different appData folder, thus losing user data including cookies. This PR leaves the `--inject` fixes of #1176, but reverts the appName logic to the pre-#1176 code.

…1162) * Add ability to inject multiple css/js files * API doc: Move misplaced macOS shortcuts doc (PR nativefier#1158) When I added this documentation originally, I guess I placed it in the wrong location. * README: use quotes in example, to divert users from shell globbing pitfalls Follow-up of nativefier#1159 (comment) * Support opening URLs passed as arg to Nativefied application (fix nativefier#405) (PR nativefier#1154) Co-authored-by: Ronan Jouchet <ronan@jouchet.fr> * macOS: Fix crash when using --tray (fix nativefier#527), and invisible icon (fix nativefier#942, fix nativefier#668) (nativefier#1156) This fixes: 1. A startup crash on macOS when using the `--tray` option; see nativefier#527. ![image](https://user-images.githubusercontent.com/22625791/115987741-99544600-a5b6-11eb-866a-dadb5640eecb.png) 2. Invisible tray icon on macOS; see nativefier#942 and nativefier#668. ![image](https://user-images.githubusercontent.com/22625791/115988276-24364000-a5b9-11eb-80c3-561a8a646754.png) Co-authored-by: Ronan Jouchet <ronan@jouchet.fr> * API.md / --widevine: document signing apps to make some sites like HBO Max & Udemy work (fix nativefier#1147) * Prompt to confirm when page is attempting to prevent unload (nativefier#1163) Should alleviate part of the issue in nativefier#1151 * Add an option to upgrade an existing app (fix nativefier#1131) (PR nativefier#1138) This adds a `--upgrade` option to upgrade-in-place an old app, re-using its options it can. Should help fix nativefier#1131 Co-authored-by: Ronan Jouchet <ronan@jouchet.fr> * Bump to Electron 12.0.5 with Chrome 89.0.4389.128 * Add newly discovered Google internal login page (nativefier#1167) * Fix Widevine by properly listening to widevine-... events, and update docs (fix nativefier#1153) (PR nativefier#1164) As documented in nativefier#1153, for Widevine support to work properly, we need to listen for the Widevine ready event, and as well for certain sites the app must be signed. This PR adds the events, and as well adds better documentation on the signing limitation. This may also help resolve nativefier#1147 * Improve suffix creation + tests * API: clarif in existing doc by the way * Typo Co-authored-by: Ronan Jouchet <ronan@jouchet.fr> Co-authored-by: Ben Curtis <github@nosolutions.com> Co-authored-by: Fabian Wolf <22625791+fabiwlf@users.noreply.github.com>

…tivefier#1176) (PR nativefier#1179) This is a follow-up of nativefier#1162 (comment) PR nativefier#1162 introduced a new `generateRandomSuffix` helper function, used it for its needs (avoiding collisions of injected js/css). But it also replaced existing appname normalizing logic with it, introducing randomness in a place that used to be deterministic. As a result, starting with 0e58f8e / v43.1.0, re-creating an app would cause the app to use a different appName, thus a different appData folder, thus losing user data including cookies. This PR leaves the `--inject` fixes of nativefier#1176, but reverts the appName logic to the pre-nativefier#1176 code.

ronjouch changed the title ~~Add ability to inject multiple css/js files~~ Fix injecting multiple css/js files (fix #458) Apr 29, 2021

ronjouch reviewed Apr 29, 2021

View reviewed changes

src/build/prepareElectronApp.ts Outdated Show resolved Hide resolved

TheCleric and others added 12 commits April 29, 2021 14:16

Add ability to inject multiple css/js files

e995778

API doc: Move misplaced macOS shortcuts doc (PR nativefier#1158)

5c4ef13

When I added this documentation originally, I guess I placed it in the wrong location.

README: use quotes in example, to divert users from shell globbing pi…

40bf034

…tfalls Follow-up of nativefier#1159 (comment)

Support opening URLs passed as arg to Nativefied application (fix nat…

f5aa854

…ivefier#405) (PR nativefier#1154) Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

API.md / --widevine: document signing apps to make some sites like HB…

0c87799

…O Max & Udemy work (fix nativefier#1147)

Prompt to confirm when page is attempting to prevent unload (nativefi…

b439889

…er#1163) Should alleviate part of the issue in nativefier#1151

Add an option to upgrade an existing app (fix nativefier#1131) (PR na…

e8b0e3d

…tivefier#1138) This adds a `--upgrade` option to upgrade-in-place an old app, re-using its options it can. Should help fix nativefier#1131 Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

Bump to Electron 12.0.5 with Chrome 89.0.4389.128

4991055

Add newly discovered Google internal login page (nativefier#1167)

37a621c

Improve suffix creation + tests

92b9e83

TheCleric force-pushed the feature/multi-inject branch from 8b3d803 to 92b9e83 Compare April 29, 2021 18:29

Merge branch 'master' into feature/multi-inject

1ba67ef

ronjouch approved these changes Apr 30, 2021

View reviewed changes

ronjouch added 2 commits April 30, 2021 06:10

API: clarif in existing doc by the way

232b039

Typo

d01cf27

TheCleric merged commit dd6e15f into nativefier:master Apr 30, 2021

TheCleric deleted the feature/multi-inject branch April 30, 2021 15:04

mattruzzi mentioned this pull request May 1, 2021

[regression-43.1.0] Injected js broken, with app error "Cannot find module loglevel" #1175

Closed

4 tasks

ronjouch mentioned this pull request May 3, 2021

Fix logging out users on upgrade / app recreate with same URL (fix #1176) #1179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix injecting multiple css/js files (fix #458) #1162

Fix injecting multiple css/js files (fix #458) #1162

TheCleric commented Apr 27, 2021

TheCleric commented Apr 29, 2021

ronjouch left a comment •

edited

Loading

ronjouch Apr 30, 2021

TheCleric Apr 30, 2021

ronjouch Apr 30, 2021

Fix injecting multiple css/js files (fix #458) #1162

Fix injecting multiple css/js files (fix #458) #1162

Conversation

TheCleric commented Apr 27, 2021

TheCleric commented Apr 29, 2021

ronjouch left a comment • edited Loading

Choose a reason for hiding this comment

ronjouch Apr 30, 2021

Choose a reason for hiding this comment

TheCleric Apr 30, 2021

Choose a reason for hiding this comment

ronjouch Apr 30, 2021

Choose a reason for hiding this comment

ronjouch left a comment •

edited

Loading