bem / webpack-bem-loader

Webpack BEM loader
Other
25 stars 14 forks source link

Custom generators and possibility to configure order of techs #60

Closed Yeti-or closed 6 years ago

Yeti-or commented 6 years ago

closes #56 closes #58

Yeti-or commented 6 years ago

@nikita-ezhkov @golyshevd take a look please

nikita-ezhkov commented 6 years ago

But does not work for me. I am investigating..

nikita-ezhkov commented 6 years ago

@Yeti-or @golyshevd It seems that it works in pair with bem-core because of this line https://github.com/bem/webpack-bem-loader/blob/master/generators/js.js#L11, but it does not work with "general" generator with no magical property of imported object. It practically avoids value of my PR. But it has worked.

I have made some examples. The branch with #56 https://github.com/nikita-ezhkov/webpack-bem-loader.example/tree/possibility-of-generators-customization - works fine. The branch with #60 https://github.com/nikita-ezhkov/webpack-bem-loader.example/tree/custom-generators - does not work as well

veged commented 6 years ago

@nikita-ezhkov sorry, still miss the point with js: null... why you wanna use it even if you can write your custom generator with all features needed? you can write analog of https://github.com/bem/webpack-bem-loader/blob/master/generators/js.js#L11 but without unneeded specific

nikita-ezhkov commented 6 years ago

@veged Sure, but it looks weird. I have not got any logic here. As user of the webpack loader i just want to get expanding of BEM entities. But with this PR i have to write non-clearly custom function to make it work. Moreover, i ought to design a mechanism of sharing this code base between all my projects to avoid copy-paste. It is a way, naturally, but it does not looks user friendly.

I would love to make behaviour which is reached with js: null to default, but it would be breaking changes. So for my opinion js: null is a setting to default, resetting of feature, but own generator is an additional logic.

veged commented 6 years ago

@nikita-ezhkov

I've got your point

which way of BEM entities expanding you imply as default? cause now we consider that applyDecl functionality needed even for non-react type of JS (if you not want to pollute global scope)

nikita-ezhkov commented 6 years ago

@veged sorry for the delay.

I would say that https://github.com/bem/webpack-bem-loader/blob/master/generators/general.js is to be a default generator out of the box, but https://github.com/bem/webpack-bem-loader/blob/master/generators/js.js is to be used for bem-core (option or some thing else). It seems to be more predictably for small projects which is not require any frameworks (i mean bem-core) or for any migration period. Apart from it, ideologically, it seems to be a good practice to do loader independ from implementation.

It has worked fine while JS was the last tech. I would keep the return value for this case.

nikita-ezhkov commented 6 years ago

Made a PR https://github.com/bem/webpack-bem-loader/pull/61/files. Is it okay for you?

cc @Yeti-or @golyshevd

veged commented 6 years ago

@nikita-ezhkov unfortunately JS can't be last tech since main dependencies gonna be resolved from JS — so if we have select which is dependent on button, than we need [button.js, button.css, select.js, select.css] (for proper CSS priority) and if we do JS last then order will be [select.css, button.css, button.js, select.js]

but, if you want to, you can configure order of techs in config (to make [css, js])

veged commented 6 years ago

@nikita-ezhkov and, after thinking, I agree with default generator https://github.com/bem/webpack-bem-loader/blob/master/generators/general.js and https://github.com/bem/webpack-bem-loader/blob/master/generators/js.js for cases with applyDecls — we just need more abstract naming for second one, since it's actually not strictly dependent on bem-react-core and can be used with other libs

nikita-ezhkov commented 6 years ago

@veged

unfortunately JS can't be last tech since main dependencies gonna be resolved from JS

I see. It should be so with PR https://github.com/bem/webpack-bem-loader/pull/61/files too. I have answered this topic https://github.com/bem/webpack-bem-loader/pull/61#discussion_r163308496

nikita-ezhkov commented 6 years ago

@Yeti-or So, what are we going to do here?

Yeti-or commented 6 years ago

@nikita-ezhkov we are going to merge =) just checking that it is working on our sources

Yeti-or commented 6 years ago

@nikita-ezhkov thanx for patience @golyshevd thanx for spotlighting huge error