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: drop css modules #748

Merged
merged 45 commits into from
Aug 20, 2018
Merged

refactor: drop css modules #748

merged 45 commits into from
Aug 20, 2018

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Jul 9, 2018

Briefly

Bug fixes

  • Add file from error to dependencies graph if something throw error (if you have broken css code on first watch run, before it requires restart watching, now it is no longer necessary)
  • Escaped characters now works correctly

Features

  • Reuse ast from postcss-loader (if used), it is decrease build time
  • API for modify generated code (i.e. allows to import and export what you want, also allows to modify generated code)
  • Validate options

Breaking changes

  • Removed modules option, use postcss-modules plugin with postcss-loader
  • Removed localIdentName option
  • Removed camelCase option

BREAKING CHANGE: use `postcss-modules` plugin with `postcss-loader`
@codecov
Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #748 into master will decrease coverage by 1.3%.
The diff coverage is 96.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
- Coverage   98.25%   96.95%   -1.31%     
==========================================
  Files          10        6       -4     
  Lines         344      197     -147     
  Branches       76       57      -19     
==========================================
- Hits          338      191     -147     
  Misses          6        6
Impacted Files Coverage Δ
lib/SyntaxError.js 100% <100%> (ø)
lib/plugin.js 100% <100%> (ø)
lib/runtimeEscape.js 100% <100%> (ø)
lib/loader.js 91.22% <91.22%> (-8.78%) ⬇️
lib/runtime.js 97.5% <97.5%> (ø)

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 43179a8...81d104a. Read the comment docs.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 8, 2018

I'm not agreeing with that change as the new boilerplate adds unneeded sections

  • Requirements (solved by badges)
  • Contributing (@see CONTRIBUTING.md)
  • License (@see LICENSE.md and Github UI)

and removes useful additions like the options table as overview and to navigate, also

Woke (old README)

woke

Broke (new README)

broke

more repos use the 'old' style atm, I don't know who wants to update all of them, shellscape (who did most of the recent updates) doesn't seemed to be active anymore (?). Now we have again an inconsistent mix here again...

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 8, 2018

@michael-ciniawsky Let's discuss this in webpack-defaults, others also take part in the discussion. We often re-do the same thing several times and lose time because we can not agree among ourselves, let's not do it again.

Once we decide how it should look, we will immediately update README. I'm not against these changes, I'm against doing it in different repositories instead in one (webpack-defaults).

Feel free to open issue and ping me.

@michael-ciniawsky
Copy link
Member

kk, lets discuss it separately

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 8, 2018

How are we going to landing this ? Will you rebase and reword/squash the commits for the CHANGELOG and we rebase merge it or should we squash merge it as one commit, which would be ok for the BREAKING CHANGES but loses the features and fixes mentioned above (first comment) 😯 ?

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 8, 2018

⚠️ We need to branch out a 1.0.0 from the master branch beforehand aswell

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 8, 2018

How are we going to landing this ? Will you rebase and reword/squash the commits for the CHANGELOG and we rebase merge it or should we squash merge it as one commit, which would be ok for the BREAKING CHANGES but loses the features and fixes mentioned above (first comment) hushed ?

I will squash all commits and write CHANGELOG before release (because conventional commits appears only here).

warning We need to branch out a 1.0.0 branch beforehand aswell

Why? We already have tag https://github.com/webpack-contrib/css-loader/tree/v1.0.0, no need branch. Anyway if you think it is ok, feel free to do this 👍

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 8, 2018

Because we will likely have to support the 1.x line for some time because of CSS Modules ? How to do a release there otherwise ? Maybe I'm missing something, not too familiar supporting and releasing multiple major release lines...

@alexander-akait
Copy link
Member Author

alexander-akait commented Aug 8, 2018

@michael-ciniawsky yep, you are right, let's create version-1 branch

done https://github.com/webpack-contrib/css-loader/tree/version-1

@alexander-akait
Copy link
Member Author

@michael-ciniawsky i will merge this PR in next branch, but not release, i have some ideas how we can improve import/export api and save compatibility with icss (new generation css-modules).

@alexander-akait alexander-akait changed the title [WIP] refactor: drop css modules refactor: drop css modules Aug 20, 2018
@alexander-akait alexander-akait merged commit bdbbbd6 into next Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants