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

General refactor #31

Closed
wants to merge 10 commits into from
Closed

General refactor #31

wants to merge 10 commits into from

Conversation

rmp135
Copy link

@rmp135 rmp135 commented Feb 26, 2017

I initially went to fix a bug with inject-loader breaking methods that were named "require" (e.g. electron.require("...")) and one thing led to another...

Anyway, the codebase can be tested in a much cleaner way with a lot less plumbing between the various parts. The tests have been fleshed out and unnecessary dependencies trimmed.
The original webpack1 and webpack2 integration tests are still in place and pass.

"babel-plugin-add-module-exports" plugin adds the module.exports property onto exported babel files with a single default export.
This allows interoperability between Babel 6 and Webpack 1.
Refactored the old code to allow for more granular unit testing.
Removed much of the plumbing that wasn't necessary.
Added more flow annotations and JSDoc comments.
Fixed bug: Do not replace properties with the name `require` e.g. `electron.require("lib")`.
This looks a little messy but allows for running the tests on all OSs with the choice of reinstalling dependencies.
Probably should have done this when I bumped the config version.
This file is used by yarn to ensure that all developers are using the correct dependency versions.
Also fixed a test to actually test what it  was meant to test.
@plasticine
Copy link
Owner

@rmp135 Hey mate — thanks for the PR! Have given it a quick look over, looks like a really nice little cleanup! :)

Will try and find some time to pull it down properly and have a look in more detail today.

@rmp135
Copy link
Author

rmp135 commented Mar 8, 2017

What's the status on this?

@plasticine
Copy link
Owner

@rmp135 Sorry — I’ve been AFK for a bit and have not got around to it yet.

@vladimir-tikhonov
Copy link
Collaborator

@rmp135 seems like everything you've done here is covered by #36. I'll close this pull request, but feel free to re-open it if I missed something.

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