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

refactor: add webpack-defaults #34

Closed
wants to merge 6 commits into from
Closed

refactor: add webpack-defaults #34

wants to merge 6 commits into from

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Apr 8, 2017

Fix

  • Error Message

Test

  • Add Tests

Docs

  • Add LOADER.md (JSDoc)
  • Update README.md

Refactor

  • Add webpack-defaults
  • Support CoffeeScript v2
  • Load Sourcemaps only if setup (options.sourceMap)

TODO

  • add schema-utils

Fixes #14, #21, #31, #32, #33

@michael-ciniawsky
Copy link
Member Author

The ESLint Config is interpreted a bit tbh 😛 , but I would like to open discussion about a few of these rules anyways import/order, import/no-dynamic-require, global-require, no-param-reassign

@@ -0,0 +1,27 @@
const fs = require('fs-extra');
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd just disable eslint for any test utility files, they have little to no bearing on code quality where it really matters.

Copy link
Member

Choose a reason for hiding this comment

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

To the point, the rules you mentioned in the comment above have an effect on code readability / quality for source files and are there for a reason imo.

For something like webpack.build.js where it has no effect on the above, I would sooner disable the linting outright in the file than disable rules across the organization that have a positive effect on overall quality.

Copy link
Member

@joshwiens joshwiens Apr 8, 2017

Choose a reason for hiding this comment

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

global-require should not be disabled by default as it's frowned upon in general.

In the certain cases where it's necessary, it should be disabled inline or in the .eslintrc on a case by case basis and set to warn from error so they can be addressed at a later date where applicable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still import/order no-param-reassign are 'conflicting' with the loader logic, the first for style/readability the latter for sourcemaps

import fs from 'fs'
import path from 'path'

import { getOptions } from 'loader-utils'
import { validateOptions } from 'schema-utils'

import helper from './helper'
if (options.sourceMap) {
  map = updateMap(map)
}

Copy link
Member

Choose a reason for hiding this comment

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

@joshwiens joshwiens changed the title v1.0.0 refactor: Apply webpack-defaults Apr 8, 2017
@michael-ciniawsky michael-ciniawsky changed the title refactor: Apply webpack-defaults refactor: add webpack-defaults Apr 9, 2017
"main": "dist/cjs.js",
"files": [
"dist"
],
Copy link
Member Author

Choose a reason for hiding this comment

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

"repository": {
    "type": "git",
    "url": "git+https://github.com/webpack-contrib/coffee-loader.git"
  },
  "bugs": {
    "url": "https://github.com/webpack-contrib/coffee-loader/issues"
  },
  "homepage": "https://webpack.js.org",

@joshwiens
Copy link
Member

@michael-ciniawsky - As we work through these, just create a named branch in the origin repo, makes it easier to test out for the group. You have org wide write access, far simpler than maintaining a fork :)

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