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

chore(package): v3.0.0 #384

Merged
merged 17 commits into from Aug 8, 2018
Merged

chore(package): v3.0.0 #384

merged 17 commits into from Aug 8, 2018

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Aug 4, 2018

Notable Changes

Commit Message Summary (CHANGELOG)

BREAKING CHANGE: requires `node >= v6.0.0`

Type

  • Chore

Issues

SemVer

  • Breaking Change (:label: Major)

Checklist

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@coveralls
Copy link

coveralls commented Aug 4, 2018

Coverage Status

Coverage increased (+7.07%) to 87.302% when pulling 1d496f6 on develop into 114db12 on master.

src/index.js Outdated

const parseOptions = require('./options')
const validateOptions = require('schema-utils')
const { getOptions } = require('@webpack-utilities/loader')
Copy link
Member

Choose a reason for hiding this comment

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

Don't use this package, we don't have time support this package, also he use own logic for parse query, in webpack@5 logic will be change in some places (loader-utils will be update too).

You will not have time to rewrite the logic, you already started a lot of PR and abandoned their. Does rational things, if you understand that you will not be able to lead the package in the future, use the standard api (loader-utils).

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR Due to experience with the ecosystem I do prefer to have 'control' over webpack deps I need and use by now, but in this case I'm open to eventually revert that change as it isn't optimal either for reasons you already mentioned...

Is there any discussion/documentation about the (planned) changes to the Loader (Runner) API ? (Guessing no)


The problem is that I'm fairly sceptical by now, if relying on webpack/webpack-contrib modules is a safer choice long-term then maintaining my own. Take webpack-contrib/schema-utils as an example here, where someone introduces breaking changes to the module then seemed to have noticed that webpack already uses that module in it's current form and finally concludes stupidly to release a new package (@webpack-contrib/schema-utils) as a result without giving a shit and with no easy way to at least maintain the 'old' version already in use. I'm not getting started on the unnecessary changes made there.... I'm simply sick and tired of working on xyz and at the end of the day it is for nothing concrete because of organisational issues/no feedback and/but on the other hand to top it still every time someone bluntly chimes in and does his/her own thing anyways causing nothing but havoc and burden. I'm not 'for fun' forking and yet planning to release/releasing modules of my interest + x under @webpack-utilties/*, @webpack-loader/* and @webpack-plugins/* again where 'needed'. It took my ~2 days to cleanup the useless bloat (e.g webpack-defaults madness) for my pending PR's and get those changes of mine in compared to partaitly months of fruitless discussion resulting in stalled PR's in zero progress one way or the other. (It's for e.g not really true that I abandoned most of my PR's some where also simply closed depsite the fact that they were basically ready but may have contained breaking changes like a higher node version (because of webpack-defaults) I sometimes simply left them since there was no pressure or need to immediately release them). Anyways... my main intention for contributing was to cleanup and simplify the horribly redundant loader and plugin landscape wherever possible, but it more and more turns out that this seems to be impossible within that ecosystem and is even getting worser every day (Mutliple CLIs, Mutliple devServer's etc). I'm not interested in that and to some point I already simply sticked the finger to it

Note that the issues and complains made above do explicitly not apply to you

Copy link
Member

Choose a reason for hiding this comment

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

Is there any discussion/documentation about the (planned) changes to the Loader (Runner) API ? (Guessing no)

Only in private messages

In fact, it is only now that work has begun on who is responsible for what, although this should have been done much earlier. And frankly it's bad that we lost you as contributor, I would be glad to see if you would return back.

I'm simply sick and tired of working on xyz and at the end of the day it is for nothing concrete because of organisational issues/no feedback and/but on the other hand to top it still every time someone bluntly chimes in and does his/her own thing anyways causing nothing but havoc and burden.

Right now I perform this function and opened for any communication and feedback.

I know that we have a lot of legacy stuff in many loaders/utils/plugins, which should be removed/refactored, and feel free to do this. Also webpack-default is just a repository that helps make the code more consistently. We do not necessarily have to release a huge update (webpack-default + breaking change) in one major release - the most important thing is stable code.

@alexander-akait
Copy link
Member

Also can you provide here https://github.com/postcss/postcss-loader/blob/7e3a95624aeba0634ca9fac178805e052a02df3b/src/index.js#L161 postcss version, future postcss release can have difference ast, i will release new version of css-loader in near future, will be great use ast only when postcss version is same

@michael-ciniawsky
Copy link
Member Author

6bf774f

@alexander-akait
Copy link
Member

@michael-ciniawsky thanks!

@michael-ciniawsky
Copy link
Member Author

Should we use the semver major part of the PostCSS version for the ast version and make it a {Number} instead?

{
-  version: '7.0.2'
+  version: 7
}

@alexander-akait
Copy link
Member

@michael-ciniawsky i think better return full version, where necessary I can check only the major version myself, also in theory minor version can change ast too

@alexander-akait
Copy link
Member

/cc @michael-ciniawsky let's finish this PR asap and then continue working on css-loader, because I want to use postcss@7 in css-loader

src/Error.js Outdated

this.message = ''
this.message += `${this.name} \n\n(${err.line}:${err.column}) ${err.reason}`
this.message += `${this.name} \n\n(${line}:${column}) ${reason}`

This comment was marked as resolved.

This comment was marked as resolved.

src/Warning.js Outdated

const { text, line, column } = warning

this.name = 'LoaderWarning'

This comment was marked as resolved.

This comment was marked as resolved.

src/options.json Outdated
"exec": {
"type": "boolean"
},
"ident": {

This comment was marked as resolved.

This comment was marked as resolved.

src/Warning.js Outdated
const { text, line, column } = warning

this.name = 'LoaderWarning'
this.message = `\n(${line}:${column}) ${text}\n`
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I want consistent formatting with the current {Error} Format and the toString() message looks weird imho

Warning

{{ Name }}
\n
({{ Line }}:{{ Column }}) {{ Text }}
\n
Warning 

(1:1) I'm a Warning

(Syntax)Error

{{ Name }}
\n
({{ Line }}:{{ Column }}) {{ Message }}
\n
{{ Code Frame }}
\n
SyntaxError

(1:1) I'm an Error

1 |  
2 >|  .class 
3 |

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky 👍 but you should check line and column anyway, because not all warnings contain this

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not sure about that and took the risk for now, will update

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky also using own warning text message can lead to loss of information (https://github.com/postcss/postcss/blob/master/lib/warning.es6#L74)

Original

postcss-lint:a.css:10:14: Avoid !important

You lose plugin name, you lose original source file path. Be careful postcss warning is not error, it is just text message, better use postcss logic for this instead own. There is nothing to worry about that they have difference view. Remember - we just wrapper and should provide easy interface. In next version of postcss message properties can be changes and it is require modify loader source code, it is not good idea.

src/Warning.js Outdated

this.name = 'Warning'

if (line && column) {

This comment was marked as resolved.

@michael-ciniawsky michael-ciniawsky changed the title [WIP] chore(package): v3.0.0 chore(package): v3.0.0 Aug 8, 2018
@michael-ciniawsky michael-ciniawsky merged commit 313c3c4 into master Aug 8, 2018
@michael-ciniawsky michael-ciniawsky deleted the develop branch August 8, 2018 17:36
@michael-ciniawsky michael-ciniawsky removed this from Chore in Dashboard Aug 8, 2018
@michael-ciniawsky michael-ciniawsky removed this from the 3.0.0 milestone Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants