refactor: apply `webpack-defaults` #130

bebraw · 2017-03-05T14:45:21Z

What kind of change does this PR introduce?

Refactoring.

Did you add tests for your changes?

The old tests should work.

Summary

This PR integrates webpack-defaults to the project.

Does this PR introduce a breaking change?

Yes. Will enforce NodeJS > 4.3 via engines.

Must release as semver MAJOR

Closes #148

codecov · 2017-03-05T14:49:06Z

Codecov Report

❗ No coverage uploaded for pull request base (master@46cb916). Click here to learn what that means.
The diff coverage is 85%.

@@           Coverage Diff           @@
##             master   #130   +/-   ##
=======================================
  Coverage          ?    85%           
=======================================
  Files             ?      2           
  Lines             ?     40           
  Branches          ?     18           
=======================================
  Hits              ?     34           
  Misses            ?      6           
  Partials          ?      0

Impacted Files	Coverage Δ
src/cjs.js	`0% <0%> (ø)`
src/index.js	`87.17% <87.17%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46cb916...e9a7aab. Read the comment docs.

michael-ciniawsky

👍

michael-ciniawsky · 2017-03-05T15:38:22Z

src/index.js

+    this.emitFile(outputPath, content);
+  }
+
+  return `module.exports = ${publicPath};`;


// webpack 2 if (loader.version === 2) return `export default ${publicPath}` return `module.exports = ${publicPath}`

That's original code I moved over. Looks like it detects webpack version. This is actually something I can do in certain other place...

Yeah, let's do the change you proposed in another PR. 👍

Yep agreed, needs to be tested properly, it bited me yesterday in postcss-loader+ style-loader || extract-text-plugin where es2015 modules don't work atm

michael-ciniawsky · 2017-03-05T15:41:34Z

package.json

    "standard-version": "^4.0.0",
-    "mocha": "~1.21.3"
+    "webpack-defaults": "^0.1.1"
  },
  "scripts": {


Please sort them 😛 e.g

"lint": "eslint --cache src test", "test": "jest", "test:coverage": "jest --collectCoverageFrom='src/**/*.js' --coverage", "travis:lint": "yarn run lint && yarn run nsp", "travis:test": "npm run test", "travis:coverage": "yarn run test:coverage"

Any thoughts on this @sapegin? Is this something we can fix at mrm?

scripts aren't reordered automatically so should be easy enough, i'll PR it into defaults in a minute.

Not sure would be the best approach here. Sorting everything (including existing scripts) feels too destructive. Also would be nice to do it not just for scripts. @bebraw Good feature for webpack-merge? ;-)

I'm not sure. The language specification doesn't really say anything about key order if I remember right.

Let's skip this problem for now. Open an issue at webpack-defaults repo, though.

Note that npm has a tendency to do its own sorting when it touches package.json somehow so I'm not sure if it's worthwhile to spend too much time on fixing this.

joshwiens

I would personally like to get people accustomed to using import | export | export default and so on.

The Babel config provides access to all the ES module stuff & it doesn't transpile into a book, may as well make use of it now and get people into the esNext groove :)

joshwiens · 2017-03-05T15:48:55Z

test/optional-file-emission.test.js

@@ -1,29 +1,27 @@
-var should = require("should");
-var fileLoader = require("../");
+const fileLoader = require('../');


import fileLoader from '../src';

Yeah, I'll port those. 👍

joshwiens · 2017-03-05T15:50:39Z

src/index.js

+  MIT License http://www.opensource.org/licenses/mit-license.php
+  Author Tobias Koppers @sokra
+*/
+const loaderUtils = require('loader-utils');


import loaderUtils from 'loader-utils';

joshwiens · 2017-03-05T15:50:53Z

src/index.js

+*/
+const loaderUtils = require('loader-utils');
+
+module.exports = function fileLoader(content) {


export default function fileLoader(content) {

joshwiens · 2017-03-05T15:52:24Z

src/index.js

+
+  return `module.exports = ${publicPath};`;
+};
+module.exports.raw = true;


export const raw = true;

joshwiens · 2017-03-05T15:54:17Z

test/correct-filename.test.js

-var should = require("should");
-var path = require("path");
-var fileLoader = require("../");
+const fileLoader = require('../');


import fileLoader from '../src';

joshwiens · 2017-03-05T16:51:25Z

test/correct-filename.test.js

-var should = require("should");
-var path = require("path");
-var fileLoader = require("../");
+const fileLoader = require('../');


Also needs to be moved into the src folder. Tests should reside with the code they are testing ( or that is what was discussed ).

The build task ignores them though we need to put thought into how we want to handle libs with extensive fixtures

Ah, yeah. I forgot to do that. Good call.

Maybe we end up with both. Unit tests below src and fixtures/integration tests below test.

Looking at some of the libs, maybe a test folder is going to be the cleaner way to do this. Jest is smart enough to handle the preprocessing regardless of where the tests reside.

joshwiens · 2017-03-05T16:58:00Z

src/index.js

+  }
+
+  return `module.exports = ${publicPath};`;
+};


Shouldn't be a semi here

I think that was forced by the linting rules.

I promise that will fail if you run yarn run lint

It's actually the opposite then:

.../file-loader/src/index.js 58:2 error Missing semicolon semi

It's because of module.exports = function fileLoader(content) {...};.

joshwiens · 2017-03-05T20:32:43Z

I pulled down the branch and made the suggested changes so I could check the transpiled output.

Also probably best to hold off until the following PR's land and are published.

webpack-contrib/webpack-defaults#15
webpack-contrib/webpack-defaults#17

bebraw · 2017-03-06T06:18:33Z

@d3viant0ne Ok, feel free to force push your changes over the PR. 👍

joshwiens · 2017-03-07T01:04:34Z

@bebraw @sapegin - This is ready for a final look.

I've made the updates discussed above.
Updated & published [email protected]
Removed all the Bithound webhooks in their application across the org.

Reminder: As NodeJS 4 support hasn't been enforced for this loader yet, this will have to be semver MAJOR. Squash & merge with the following message to trigger a major in standard-version

feat(defaults): Integrate webpack-defaults

BREAKING CHANGE: Enforces a NodeJS version > 4.3

bebraw · 2017-03-07T08:05:28Z

The only part that's a little puzzling to me is the handling of imports. Example:

file-loader [feat-webpack-defaults] $ npm run build

> [email protected] prebuild .../file-loader
> yarn run clean:dist

yarn run v0.20.3
$ del-cli dist
✨  Done in 0.50s.

> [email protected] build .../file-loader
> cross-env NODE_ENV=production babel -s true src -d dist --ignore 'src/**/*.test.js'

src/index.js -> dist/index.js
file-loader [feat-webpack-defaults] $ node
> require('./')
.../file-loader/dist/index.js:5
import loaderUtils from 'loader-utils';
^^^^^^
SyntaxError: Unexpected token import

It might make sense to force preprocessing imports too and expose through module.exports. Basically Node's lack of ES6 module support bites right here.

This isn't an issue if you use the loader within webpack, but it will be problematic if you use loader-runner and such.

joshwiens · 2017-03-07T08:30:30Z

Open an issue in defaults so we can figure out the correct output for "all the things"

bebraw · 2017-03-07T08:33:14Z

Sure. 👍

bebraw · 2017-03-07T08:35:57Z

webpack-contrib/webpack-defaults#23 for further discussion.

sapegin · 2017-03-07T08:49:22Z

.eslintrc

@@ -0,0 +1,6 @@
+{
+  "extends": "webpack",
+  "globals": {


I think we don’t need this anymore, since webpack preset includes jest environment.

Yeah, safe to drop. 👍

joshwiens · 2017-03-07T09:09:18Z

@bebraw this should transpile down properly. Still needs to have the version bumped once the next patch of defaults is released but the actual change is applied in 35ad7fa

bebraw · 2017-03-07T09:19:01Z

Yup. Basically

file-loader [feat-webpack-defaults] $ node
> require('./')
{ raw: true, default: [Function: fileLoader] }

now. This leaves one decision - how to handle default.

bebraw · 2017-03-09T07:42:59Z

Ready for a review. I changed the module entry point as discussed at webpack-contrib/webpack-defaults#23.

joshwiens · 2017-03-09T16:35:44Z

src/index.js

+
+  return `export default = ${publicPath};`;
+}
+export const raw = true;


One thing of note, we are going to need a consistent way to handle the proxy + libs. While it seems like overkill on a single file library like file-loader, applying the same to something like css-loader would be a bit messy.

Proposing we as a standard, leave cjs.js in ./src and move any library files into ./src/lib

Another way, go with _cjs.js so at least it will stand out as glue code. No need for src/lib then.

Still doesn't solve the issue with the more complicated libs imo. For the sake of simplicity, particularly with defaults upgrade / update still think libs in lib would be cleaner. Not that I want to endlessly debate the topic.

Take a look at some of our more complicated loaders / plugins, just hammering everything into ./src stands to get a little messy.

michael-ciniawsky · 2017-03-31T11:50:26Z

package.json

@@ -4,20 +4,49 @@
  "author": "Tobias Koppers @sokra",
  "description": "file loader module for webpack",
  "files": [
-    "index.js"
+    "index.js",


This can be removed if webpack-defaults > v0.4.4

bebraw · 2017-03-31T11:53:22Z

I would say merge, publish a 1.0 beta.

joshwiens · 2017-03-31T21:06:18Z

@bebraw - A thought while I'm working on the exports-loader upgrade and it's tests, we had discussed testing everything through webpack which is something i'm doing in exports-loader.

We should really make it a point to either create an integration suite or just test everything through webpack to include file-loader.

bebraw · 2017-04-01T05:18:30Z

@d3viant0ne Yeah, now that I think of it, testing directly through webpack is the sanest option. It adds some overhead but at least that avoids a category of bugs and integration tests are needed anyway so it's hard to avoid not running through webpack.

joshwiens · 2017-04-01T08:44:23Z

Do we want to add webpack as a peerDep or just a note in the contribution guidelines that webpack is required for local development?

bebraw · 2017-04-01T09:23:02Z

@d3viant0ne Since you are going to need it for development anyway, then a dev dep would make sense. Setting a peer dep as well would be good, though, as that would allow us to communicate version support to users (much harder to run incompatible versions).

joshwiens · 2017-04-01T09:33:26Z

devDep limits our ability to test against multiple versions in travis

bebraw · 2017-04-01T09:35:29Z

Good point. What if we push the problem to an init script a developer runs when starting to develop? That would do npm i and pull a fresh version of webpack to test against.

joshwiens · 2017-04-02T01:22:59Z

Same issue applies to local development imo. Any automated install of Webpack will be limiting in the same manner.

I think particular instance, i think clear documentation should be enough. For anyone familiar with the ecosystem, needing Webpack for loader / plugin development is obvious. For anyone that isn't, it's a simple lesson they will only have to learn once which can be at least partially mitigated in a DEVELOPMENT.md / CONTRIBUTING.md

bebraw · 2017-04-03T08:04:28Z

I think particular instance, i think clear documentation should be enough. For anyone familiar with the ecosystem, needing Webpack for loader / plugin development is obvious. For anyone that isn't, it's a simple lesson they will only have to learn once which can be at least partially mitigated in a DEVELOPMENT.md / CONTRIBUTING.md

Ok, cool. 👍

bebraw · 2017-06-05T10:58:13Z

@d3viant0ne: Do you want to move with this later? I don't mind updating the PR.

bebraw requested a review from joshwiens March 5, 2017 14:45

bebraw force-pushed the feat-webpack-defaults branch from 806aa38 to 8966cd3 Compare March 5, 2017 14:56

michael-ciniawsky approved these changes Mar 5, 2017

View reviewed changes

joshwiens suggested changes Mar 5, 2017

View reviewed changes

bebraw mentioned this pull request Mar 5, 2017

refactor: export as ES2015 Module webpack-contrib/raw-loader#10

Merged

joshwiens reviewed Mar 5, 2017

View reviewed changes

joshwiens mentioned this pull request Mar 5, 2017

feat: incorporate standard-version webpack-contrib/webpack-defaults#15

Merged

joshwiens approved these changes Mar 6, 2017

View reviewed changes

joshwiens force-pushed the feat-webpack-defaults branch from e5d3d65 to a2697af Compare March 6, 2017 21:08

michael-ciniawsky approved these changes Mar 7, 2017

View reviewed changes

bebraw mentioned this pull request Mar 7, 2017

RFC - Dealing with imports/exports webpack-contrib/webpack-defaults#23

Closed

sapegin reviewed Mar 7, 2017

View reviewed changes

michael-ciniawsky approved these changes Mar 9, 2017

View reviewed changes

joshwiens reviewed Mar 9, 2017

View reviewed changes

bebraw and others added 4 commits March 31, 2017 05:05

chore: Integrate webpack-defaults

b5e96d5

chore(package): update for defaults 0.2.0

946d54a

refactor: prefer import and export default

4ab0e1a

chore(package): update to defaults 0.2.1

998f50b

michael-ciniawsky approved these changes Mar 31, 2017

View reviewed changes

chore: remove legacy index file

e9a7aab

joshwiens changed the title ~~chore - Integrate webpack-defaults~~ refactor: Apply webpack-defaults Apr 9, 2017

michael-ciniawsky mentioned this pull request Apr 14, 2017

feat(index): add cssOutputPath option (option.cssOutputPath) #150

Closed

michael-ciniawsky added pr_action: Rework labels Apr 20, 2017

michael-ciniawsky changed the title ~~refactor: Apply webpack-defaults~~ refactor: apply webpack-defaults Apr 20, 2017

michael-ciniawsky added status: rebase and removed pr_action: Rework labels Apr 23, 2017

joshwiens added pr_action: Review semver: Major type: Refactor and removed type: chore labels May 1, 2017

michael-ciniawsky added this to the 1.0.0 milestone Jun 2, 2017

michael-ciniawsky changed the title ~~refactor: apply webpack-defaults~~ refactor: apply webpack-defaults Jun 2, 2017

joshwiens removed this from the 1.0.0 milestone Jun 5, 2017

joshwiens closed this Jun 5, 2017

Uh oh!

refactor: apply webpack-defaults #130

refactor: apply webpack-defaults #130

Uh oh!

Conversation

bebraw commented Mar 5, 2017 • edited by joshwiens Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwiens left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwiens Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwiens Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwiens commented Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

refactor: apply `webpack-defaults` #130

refactor: apply `webpack-defaults` #130

bebraw commented Mar 5, 2017 •

edited by joshwiens

Loading

codecov bot commented Mar 5, 2017 •

edited

Loading

michael-ciniawsky Mar 5, 2017 •

edited

Loading

michael-ciniawsky Mar 5, 2017 •

edited

Loading

joshwiens left a comment •

edited

Loading

joshwiens Mar 5, 2017 •

edited

Loading

joshwiens Mar 5, 2017 •

edited

Loading

joshwiens commented Mar 5, 2017 •

edited

Loading

joshwiens commented Mar 7, 2017 •

edited

Loading