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

fix(plugin-webpack): fix incorrect PRELOAD_WEBPACK_ENTRY. #635

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

tgds
Copy link
Contributor

@tgds tgds commented Nov 23, 2018

BrowserWindow preload parameter should be an absolute path. Remove prepending 'file://' from prod
value of PRELOAD_WEBPACK_ENTRY.
Properly escape backslashes in the dev value of PRELOAD_WEBPACK_ENTRY to prevent failure on Windows.

  • [ x] I have read the contribution documentation for this project.
  • [x ] I agree to follow the code of conduct that this project follows, as appropriate.
  • [x ] The changes are appropriately documented (if applicable).
  • [ x] The changes have sufficient test coverage (if applicable).
  • [- ] The testsuite passes successfully on my local machine (if applicable).
    These tests are failing, but are unrelated:
1) electron-forge API (with installer=yarn)
       after init
         after package
           make
             throws an error when the specified maker doesn't support the current platform:
     AssertionError: expected promise to be rejected with an error matching /the maker declared that it cannot run/ but got 'Could not find module with name: /Users/tom/code/electron-forge/packages/api/core/test/fixture/maker-unsupported'

  2) electron-forge API (with installer=yarn)
       after init
         after package
           make
             throws an error when the specified maker doesn't implement isSupportedOnCurrentPlatform():
     AssertionError: expected promise to be rejected with an error matching /incompatible with this version/ but got 'Could not find module with name: /Users/tom/code/electron-forge/packages/api/core/test/fixture/maker-incompatible'

  3) rebuilder
       no config
         "before all" hook:
     Error: Exited with status 1

  4) rebuilder
       with config
         "before all" hook:
     Error: Exited with status 1

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, can you add a test for this?

@@ -164,8 +164,8 @@ Your packaged app may be larger than expected if you dont ignore everything othe
if (entryPoint.preload) {
defines[`${entryPoint.name.toUpperCase().replace(/ /g, '_')}_PRELOAD_WEBPACK_ENTRY`] =
this.isProd
? `\`file://\$\{require('path').resolve(__dirname, '../renderer', '${entryPoint.name}', 'preload.js')\}\``
: `'${path.resolve(this.baseDir, 'renderer', entryPoint.name, 'preload.js')}'`;
? `\`\$\{require('path').resolve(__dirname, '../renderer', '${entryPoint.name}', 'preload.js')\}\``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this part of the ternary expression need the escaping logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need it, because the result is an expression. (Keep in mind, the webpackDefine plugin does not define the variable, it just replaces MAIN_WINDOW_PRELOAD_WEBPACK_ENTRY in the source with whatever string it's given)

In production

{
  preload: MAIN_WINDOW_PRELOAD_WEBPACK_ENTRY
}

will get transformed to

{
  preload: require('path').resolve(__dirname, '../renderer', 'MAIN_WINDOW', 'preload.js')
}

Whereas in development, the result is a string and needs to get properly escaped since

MAIN_WINDOW_PRELOAD_WEBPACK_ENTRY = `'${path.resolve(this.baseDir, 'renderer', entryPoint.name, 'preload.js')}'`

results in

{
  preload: '\basedir\renderer\main\preload.js'
}

However, it has to be

{
  preload: '\\basedir\\renderer\\main\\preload.js'
}

@tgds
Copy link
Contributor Author

tgds commented Nov 23, 2018

Sure, I'll try. A unit test, or E2E and actually spawn electron browserWindow with preload?

@malept
Copy link
Member

malept commented Nov 23, 2018

A unit test, please.

@tgds
Copy link
Contributor Author

tgds commented Nov 26, 2018

@malept, a test added

BrowserWindow preload parameter should be an absolute path. Remove prepending 'file://' from prod
value of PRELOAD_WEBPACK_ENTRY.
Properly escape backslashes in the dev value of
PRELOAD_WEBPACK_ENTRY to prevent failure on Windows.
@tgds
Copy link
Contributor Author

tgds commented Nov 26, 2018

Sorry, rebased and squashed, because I used my company email address by mistake in the original commit

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use String.raw() anywhere where there's a string literal with backslashes.

Other than that, I defer to @MarshallOfSound.

packages/plugin/webpack/src/WebpackPlugin.ts Outdated Show resolved Hide resolved
@tgds
Copy link
Contributor Author

tgds commented Dec 6, 2018

@MarshallOfSound, any chance to merge this soon, please, as this bug is blocking the use of preload scripts on Windows?

@MarshallOfSound MarshallOfSound merged commit 6eae1b5 into electron:master Dec 13, 2018
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

3 participants