search

pendo-io / pendo-designer-electron

NPM module to add Pendo Designer to Electron apps
1 stars 0 forks source link

APP-2803 make electron great and work with remote deployments #10

Closed tato123 closed 7 years ago

tato123 commented 7 years ago

APP-2803

Use remote deployment files for electron instead of local (does not include any changes to login flow since that seemed to require additional work)

This change is Reviewable

tato123 commented 7 years ago

+@4storia

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

4storia commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions.

contentScript.js, line 6 at r1 (raw file):

const version = require('./package.json').version;
const DEV_MODE = process.env.DEV_MODE;
const REMOTE_HOST = process.env.REMOTE_HOST;

how do these actually get set? is there an package.json script or something?

contentScript.js, line 28 at r1 (raw file):

            window.ipcRenderer = require('electron').ipcRenderer;
        }
        window.ipcRenderer.send('pendo-agent-detect', { host: window.pendo.HOST, version: window.pendo.VERSION});

this seems like a race condition waiting to happen, where this gets executed and then we add the listener after the fact

contentScript.js, line 60 at r1 (raw file):

        title: 'Pendo Designer',
        partition: 'persist:pendo',
        plugins: true

is this here for something?

contentScript.js, line 75 at r1 (raw file):

                const substring = details.url.substring(details.url.lastIndexOf('/') + 1);
                const redirectURL = `${REMOTE_HOST}/${substring}`;
                console.log('redirecting', details.url, 'to', redirectURL)

can we swap these to console.info, or put them behind the existing debug flag, just so they're easy to filter out?

contentScript.js, line 85 at r1 (raw file):

    designerWindow.webContents.on('dom-ready',() => {
        if (DEV_MODE) {
            designerWindow.webContents.executeJavaScript(`window.PENDO_MODE="dev";`);

does this do something in our app, or the agent?

contentScript.js, line 175 at r1 (raw file):

                    return;
                }
                console.log('[Pendo] Using agent environment', pendoOptions)

ditto on my above console comment

Comments from Reviewable

tato123 commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions.

contentScript.js, line 6 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
how do these actually get set? is there an package.json script or something?

These need to be explicitly enabled from the calling app, I've updated the electron-test-designer to set these values, I debated between this and actually enabling it as an option in the window.pendo.launchDesigner. My only thought was that in the

Example: https://github.com/pendo-io/electron-test-designer/blob/master/package.json#L9

contentScript.js, line 28 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
this seems like a race condition waiting to happen, where this gets executed and then we add the listener after the fact

Yup, you're right, I've changed the ordering icpMain registers a listener before this can now be called, so the race condition should not exist anymore

contentScript.js, line 60 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
is this here for something?

Ok: Yup, this allows loading of remote javascript even when nodeintegration is enabled(i.e. https://pendo-dev.appspot.com/designer/latest/app.bundle.js)

contentScript.js, line 75 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
can we swap these to console.info, or put them behind the existing debug flag, just so they're easy to filter out?

Done

contentScript.js, line 85 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
does this do something in our app, or the agent?

Ok. Yup, there is new logic when displaying the version number to explicitly let the developer know they are running a dev copy of the code

contentScript.js, line 175 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
ditto on my above console comment

Done

Comments from Reviewable

tato123 commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

contentScript.js, line 155 at r2 (raw file):

        response.on('end', function() {
            const src = new String(body, "UTF-8");
            const result = vm.runInThisContext(m.wrap(src))(exports, require, module, __filename, __dirname);

FYI: This is the nasty bit, because it's a javascript file instead of a json file I have to actually interpret it and append to the module.exports

Comments from Reviewable

4storia commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

contentScript.js, line 6 at r1 (raw file):

Previously, tato123 (Jonathan) wrote…
These need to be explicitly enabled from the calling app, I've updated the `electron-test-designer` to set these values, I debated between this and actually enabling it as an option in the `window.pendo.launchDesigner`. My only thought was that in the Example: https://github.com/pendo-io/electron-test-designer/blob/master/package.json#L9

I don't think you finished this comment... :P

contentScript.js, line 155 at r2 (raw file):

Previously, tato123 (Jonathan) wrote…
FYI: This is the nasty bit, because it's a javascript file instead of a json file I have to actually interpret it and append to the `module.exports`

I'm wondering if part of plugins.js can add window.pendo.SOURCES or something? or some similar avenue for attaching the sources object to the agent, since it and plugins are the only big of shared code between these

Comments from Reviewable

4storia commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

contentScript.js, line 155 at r2 (raw file):

Previously, 4storia (Cliff Hess) wrote…
I'm wondering if part of `plugins.js` can add `window.pendo.SOURCES` or something? or some similar avenue for attaching the `sources` object to the agent, since it and plugins are the only big of shared code between these

only bits of shared code* can't edit comments :{

Comments from Reviewable

tato123 commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

contentScript.js, line 6 at r1 (raw file):

Previously, 4storia (Cliff Hess) wrote…
I don't think you finished this comment... :P

Story of my life, yeah, so my thought was that adding as a process.env was consistent with how its turned on in pendo-designer-extension and also I realized that I check DEV_MODE well before we receive any info back on debug mode from the designer window

contentScript.js, line 155 at r2 (raw file):

Previously, 4storia (Cliff Hess) wrote…
only bits of shared code* can't edit comments :{

hmm possibly, however we do use the sources outside of the window object, we use it for routing in the ipcMain method which occurs inside the node / electron app, not inside the browser window

Comments from Reviewable

4storia commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

contentScript.js, line 6 at r1 (raw file):

Previously, tato123 (Jonathan) wrote…
Story of my life, yeah, so my thought was that adding as a `process.env` was consistent with how its turned on in `pendo-designer-extension` and also I realized that I check DEV_MODE well before we receive any info back on debug mode from the designer window

OK: gotcha, sounds good to me

contentScript.js, line 155 at r2 (raw file):

Previously, tato123 (Jonathan) wrote…
hmm possibly, however we do use the sources outside of the window object, we use it for routing in the `ipcMain` method which occurs inside the node / electron app, not inside the browser window

so I think adding it to window.pendo.sources means we could do the following:

This lets us avoid evaling a remote source, as well as letting us follow the same communication paradigm as the rest of the app, rather than a one-off https.get/http stream communication mechanism

Comments from Reviewable

4storia commented 7 years ago

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

contentScript.js, line 155 at r2 (raw file):

Previously, 4storia (Cliff Hess) wrote…
so I think adding it to `window.pendo.sources` means we could do the following: - add a function to `window`, similar to `addLaunchDesignerFn` or `addPendoAgentDetection` that responds to ipc requests for sources with the `window.pendo.sources` object - send a message via `ipcRenderer` for it in this file, or add a listener on `ipcMain` for it (either way) - receive the response and save it off appropriately This lets us avoid `eval`ing a remote source, as well as letting us follow the same communication paradigm as the rest of the app, rather than a one-off `https.get`/http stream communication mechanism

nvm, f2f, lets just changes the sources file to json

Comments from Reviewable