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

React 16 + HMR + Single Webpack + Reorganization #65

Merged
merged 21 commits into from
Nov 9, 2017
Merged

Conversation

Flaque
Copy link
Contributor

@Flaque Flaque commented Nov 8, 2017

Ho boy so this is gonna be a lot.

Main Rational

We need to be able to reuse code from lib in both the renderer and main process. To do that we'll need to be able to accommodate both styles of module loading:

Renderer Style imports

// ES6 module loading that works on the renderer process
import Foo from "./foo.js"

Main (or node) style imports

// Main process (node) module loading
const Foo = require("./foo.js");

On the frontend import statements work because of our webpack config. We needed to have webpack for React and happened to get import statements for free.

THE PROBLEM

Some modules should be available to both the renderer AND main. However, if they use import statements anywhere in their code (even in things they load in) they will completely fail in the main process.

So for example, let's say we have this import-style module:

// lib/someImportStyleModule.js
import Jazz from "jazz";

const Buzzwinkles =  () => {
  console.log("I am an import thing");
}

export default {
  Buzzwinkles
};

Then, later we would like to use it in the main process somewhere like so:

const SomeImportStyleModle = require("../lib/someImportStyleModule"); // This will break

This will produce an error that tells us it doesn't know what the import statement on the first line of lib/someImportStyleModule.js is.

The fix

With this problem, what we could do is add yet another webpack config to the project. This would mean yet another build command and run command to add to our... 😅 ever expanding selection:

awful amount of script commands

Since that seemed like a nightmare that I don't think we want to go down, I looked for some better options and came across electron-webpack.

Electron-webpack gives us existing webpack configs that we don't need to touch and use, similar to what I wanted sea-floor to be like. Except this one works <3

The changes

In order to use electron-webpack nicely, we had to do some reorganization and some rule changes.

  • app and lib were combined into src/lib, src/main and src/renderer.
  • Yarn is now pseduo-required. You're welcome to continue to use it, but if you run into problems with it, I'm going to tell you to use yarn. It's faster and nicer anyway <3

If you're not familiar with yarn, here's an overview:

npm install              => `yarn install` or `yarn`
npm install --save foo   => `yarn add foo`
npm install --save-dev   => `yarn add -D foo`
npm run fizz             => `yarn fizz`
npm test                 => `yarn test`
npm run test -- --watch  => `yarn test --watch`

Added side benefits

Electron-webpack also gave us some added side stuff when I set it up.

  • We now have Hot Code Reloading! No need to restart the application anymore, it will automagically recognize your changes and update them in the app.
  • We are now on React 16.

@Flaque
Copy link
Contributor Author

Flaque commented Nov 8, 2017

TODO:

  • Fix tests to pass
  • Re-add the menu items for mutations
  • Re-update electron to run frameless
  • Re-add the Draggable header
  • Re-add the Draft-JS CSS

package.json Outdated
"compile": "electron-webpack",
"test": "cross-env NODE_ENV=test ./node_modules/.bin/jest",
"test": "cross-env NODE_ENV=test jest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

? Why? This is not a good practice.

package.json Outdated
@@ -6,7 +6,7 @@
"dev": "electron-webpack dev",
"prod": "electron-webpack prod",
"compile": "electron-webpack",
"test": "cross-env NODE_ENV=test jest",
"test": "cross-env NODE_ENV=test ./node_modules/.bin/jest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay this is better then

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just trying differnet things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ook <3

@kmccrohan kmccrohan merged commit c2d3541 into develop Nov 9, 2017
@kmccrohan kmccrohan deleted the more-extensions branch November 9, 2017 02:19
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.

2 participants