Add code #1

Vittly · 2018-01-31T06:55:27Z

Need to check:

license text in package.json and README.md
readability of README.md. We need more examples from real world projects but may be for now current text is enough
other

awinogradov · 2018-02-01T14:33:08Z

src/BemPluginConfig.js

+     *   b. libraries configs (like bem-react-components) for many reasons
+     *       example: https://github.com/bem/bem-sdk/issues/261
+     */
+    __prepareRc(bemConfig, libs, self) {


Разве это не должен делать bem-config?

Кто ж его заставит... @tadatuta

Yes it should be there. Now not all wanted libraries has their bemrc and adding it (via PR for example) is not simple unfortunately. I'll do detailed comment

awinogradov · 2018-02-01T14:34:28Z

src/BemRequest.js

+const entityFromPath = require('./helpers/entityFromPath');
+
+function completeRequest(request, ctx) {
+    // TODO: move to @bem/sdk.import-notation


Заведи ишью?

сс @Yeti-or

bem/bem-sdk#275

awinogradov · 2018-02-01T14:35:52Z

src/helpers/entityFromPath.js

+const BemEntityParse = require('@bem/sdk.naming.entity.parse');
+
+function trimFilename(filePath) {
+    return path.basename(filePath).split('.')[0];


Уверен? block.react.js

Это должно быть реализовано примерно в районе @bem/sdk.naming.file.parse

path.basename('/foo/bar/b__e_m.react.js').split('.')[0]; returns 'b__e_m' @awinogradov

@zxqfox - done bem/bem-sdk#276

$ node -p "path.basename('/foo/bar/b__e_m.assets/random.png').split('.')[0];" random

And sdk.naming.file.parse would handle it ?

Feels like. But we didn't talk about it much, (bem) file here is '/foo/bar/b__e_m.assets/'

awinogradov · 2018-02-01T14:36:40Z

src/helpers/importedFiles.js

+                };
+            });
+
+            // TODO: should we add __elem (in case of __elem_mod) and _mod (in case of _mod_val) ?


Actually, not sure.

@Yeti-or Can you tell us what actually webpack-bem-loader does?

actually, it adds, why do you think it's unnecessary?

this part. I think we should simple remove this comment

Can you add an example ?

I've checked:

parse('b:block e:elem m:mod') /** * returns [ * { block: 'block', elem: 'elem' }, // __elem is here * { block: 'block', elem: 'elem', mod: { name: 'mod' } } * ] */ parse('b:block m:mod=val') /** * returns [ * { block: 'block' }, * { block: 'block', mod: { name: 'mod' } }, // _mod is here * { lock: 'block', mod: { name: 'mod', val: 'val' } } * ] */

I'll remove nasty comment

qfox · 2018-02-01T21:06:41Z

README.md

+
+## Known issues
+
+* Small tests


Not enough ? Too few/small?

maybe there are too small and too fast? we need to fix it and write big heavy ones =)

Described in #3

qfox · 2018-02-01T21:07:47Z

README.md

+## Known issues
+
+* Small tests
+* Not picking changes in `bemrc` in watch-mode


Rereading .bemrc doesn't work in watch-mode

Yes, in plugin. May be its a problem. May be we should fix it in newer version of plugin so I noted it

qfox · 2018-02-01T22:56:02Z

src/BemPluginConfig.js

+        const oldSync = rc.sync;
+
+        rc.sync = function(options) {
+            return configsMap[options.cwd] || oldSync(options);


давай напишем что тут происходит, кроме тестов?

qfox · 2018-02-01T22:58:28Z

src/BemRequest.js

+ */
+class BemRequest {
+    constructor(request, filePath, levels, setIndex) {
+        const requestNaming = namingConvention.getFromLevelsByPath(levels, filePath);


не получилось достать из bem-config?

more info in bem/bem-sdk#277

qfox · 2018-02-01T22:58:54Z

src/BemRequest.js

+class BemRequest {
+    constructor(request, filePath, levels, setIndex) {
+        const requestNaming = namingConvention.getFromLevelsByPath(levels, filePath);
+        const ctx = entityFromPath(filePath, requestNaming);


это должен быть @bem/sdk.naming.file.parse в купе с @bem/sdk.config

qfox · 2018-02-01T22:59:32Z

src/BemRequest.js

+        const ctx = entityFromPath(filePath, requestNaming);
+
+        // TODO: remove duplicates from array in @bem/sdk.import-notation
+        //   https://github.com/bem/bem-sdk/issues/263


тикет закрыт

@zxqfox я только не помню выпустили ли мы версию

qfox · 2018-02-01T23:05:46Z

src/generator/index.js

+        .sort(tech => tech === 'js') // js should be the last
+        .map(tech => {
+            const generator = tech === 'js' ? jsGenerator : generalGenerator;
+            const existingFiles = techToFiles[tech].filter(fs.existsSync);


ммм, а нельзя это делать через webpack-овские ручки?
странно работать с фс напрямую, когда webpack должен всё контролировать

Hmm. In my solution "No".

I do resolve bem-require b:b e:e by module implemented with source (require('b.js'),require('b_e.js')).applyDecls(). Lack of files causes module require error. To handle this we need to override requires of actual files. I want to avoid this and reduce amount of redefinitions of standard webpack mechanisms.

I can add a comment in a way "TODO: should do it more webpackable"

qfox · 2018-02-01T23:08:43Z

src/generator/techs/javascript.js

+'use strict';
+
+function lastRequire(file) {
+    return `require('${file}').default.applyDecls()`;


@grape, @Yeti-or мы можем перестать делать эту магию?

что ты имеешь ввиду?

я имею ввиду надо делать явное const Class = bake(decl({block: 'b'}), decl({block: 'b'}), declMod({block: 'b'}))

qfox · 2018-02-01T23:10:40Z

src/helpers/cellPath.js

+/**
+ * Returns path to file of BEM-cell using naming rules
+ */
+module.exports = (bemCell, convention) => BemCellStringify(convention)(bemCell);


точно не naming.file.stringify?

I got cell (block/elem/.../tech/layer) and level.path (absolute) and want to build path to file. Now I do level.path + .. (because layer part is already in cell) + cell

naming.file.stringify do level (any dir) + cell.

What is level ? Layer is not equal to level ?

I don't think that:

fileStringify( new BemFile({ cell, level: level.path + '/..' }) )

is better enough.

Started conversation in bem/bem-sdk#278

qfox · 2018-02-01T23:12:57Z

src/helpers/importedFiles.js

+            // TODO: should we add __elem (in case of __elem_mod) and _mod (in case of _mod_val) ?
+            //   like webpack-bem-loader does
+
+            bemDeps = bemDeps.concat(files);


[].push.apply(bemDeps, files);

why it's better ?

Do you think its faster?

qfox · 2018-02-01T23:15:45Z

src/helpers/importedFiles.js

+        });
+    });
+
+    // FIXME: does it have right order ?


Guess we can't reorder this anyway. Or you know how we can?

Yes its rhetorical somehow. I'll remove it

qfox · 2018-02-01T23:17:00Z

test/e2e/project/src/lib-with-rc/.bemrc.js

+    levels: [{ layer: 'desktop' }],
+    // TODO: remove sets after https://github.com/bem/bem-sdk/issues/262
+    sets: {
+        desktop: 'desktop'


Это нужно для того, чтобы можно было использовать слой отдельно? Но ведь это некорректно.

Agree bem/bem-sdk#262 (comment)

Yeti-or · 2018-02-04T14:52:16Z

README.md

+
+|Name|Type|Description|
+|----|----|-----------|
+|**`test`**|`{RegExp}`|Plugin processes `require` statements in matched files. Default is `/\.js$/`|


will it play nice with tech/techMap? Do we really need both?

tech is to add css

techMap is to handle .react.js

test is to skip bemhtml.js if test: /\.react\.js$/... Should we use techMap here? Is it your proposal ?

Moved to #8

Yeti-or · 2018-02-04T14:53:05Z

README.md

+
+## Known issues
+
+* Small tests


maybe there are too small and too fast? we need to fix it and write big heavy ones =)

Yeti-or · 2018-02-04T14:54:27Z

README.md

+
+> npm test
+
+## Known issues


I think we need to open an issue for each one

Yeti-or · 2018-02-04T15:17:13Z

src/BemRequest.js

+        const ctx = entityFromPath(filePath, requestNaming);
+
+        // TODO: remove duplicates from array in @bem/sdk.import-notation
+        //   https://github.com/bem/bem-sdk/issues/263


@zxqfox я только не помню выпустили ли мы версию

Yeti-or · 2018-02-04T15:18:57Z

src/generator/index.js

+
+    return Object
+        .keys(techToFiles)
+        .sort(tech => tech === 'js') // js should be the last


actually, it should be first ¯_(ツ)_/¯

nooo! why do I misunderstand it. Can you tell us what's this magic about?

wait this code converts ['js', 'css'] into ['css', 'js'] to build (require('x.css'), require('x.js').applyDecls()) so value of comma expression will be ReactComponent

I think we need to look at the example:

common.blocks/ block/ block.js block.css desktop.blocks/ block/ block.js block.css

var b = require(b:block); /** * Loader produces: * * var b = ( * require('./common.blocks/block/block.css'), * require('./desktop.blocks/block/block.css'), * ( * require('./common.blocks/block/block.js'), * ( * require('./desktop.blocks/block/block.js').default || * require('./desktop.blocks/block/block.js') * ).applyDecls() * ) * ); */ /** * Plugin produces: * * var b = ( * require('./common.blocks/block/block.css'), * require('./desktop.blocks/block/block.css'), * require('./common.blocks/block/block.js'), * require('./desktop.blocks/block/block.js').default.applyDecls() * ); */

Is there any problems? @Yeti-or

Yeti-or · 2018-02-04T15:19:27Z

src/generator/techs/javascript.js

+'use strict';
+
+function lastRequire(file) {
+    return `require('${file}').default.applyDecls()`;


что ты имеешь ввиду?

Yeti-or · 2018-02-04T15:20:32Z

src/helpers/importedFiles.js

+//   a. keeping full path in config
+//   b. allowing to build short bem-cell path without level part
+function resolveCellPath(relativePath, levelPath) {
+    return path.resolve(levelPath, '../' + relativePath);


will it work on windows?

may be we should... I'll add an issue

Yeti-or · 2018-02-04T15:21:39Z

src/helpers/importedFiles.js

+                };
+            });
+
+            // TODO: should we add __elem (in case of __elem_mod) and _mod (in case of _mod_val) ?


actually, it adds, why do you think it's unnecessary?

Yeti-or · 2018-02-04T15:22:08Z

src/helpers/importedFiles.js

+            // TODO: should we add __elem (in case of __elem_mod) and _mod (in case of _mod_val) ?
+            //   like webpack-bem-loader does
+
+            bemDeps = bemDeps.concat(files);


why it's better ?

Vittly · 2018-02-04T19:10:49Z

fixed

Vittly · 2018-02-09T09:20:31Z

I've fixed some problems and created issues for others. I want to improve readme.md on weekend and merge PR in the begining of the next week. Tell me if I wrong 👂

Vittly · 2018-02-18T09:12:18Z

I've done with readme and examples. The rest will fix later. Merge!

awinogradov reviewed Feb 1, 2018

View reviewed changes

awinogradov approved these changes Feb 1, 2018

View reviewed changes

qfox reviewed Feb 1, 2018

View reviewed changes

Vittly mentioned this pull request Feb 4, 2018

Add method to build full form of import notation bem/bem-sdk#275

Open

Yeti-or reviewed Feb 4, 2018

View reviewed changes

This was referenced Feb 4, 2018

Add method to extract level configuration from path bem/bem-sdk#277

Open

Add method to build path to file from cell + level bem/bem-sdk#278

Open

Add windows support #4

Open

Vittly force-pushed the init branch from d92ae2d to e87c9a6 Compare February 4, 2018 19:08

Vittly mentioned this pull request Feb 8, 2018

Review plugin options #8

Closed

Vittly force-pushed the init branch from e87c9a6 to f2c0630 Compare February 9, 2018 09:04

Vittly force-pushed the init branch from f2c0630 to 8de7b43 Compare February 18, 2018 09:01

Add code

f054949

Vittly force-pushed the init branch from 8de7b43 to f054949 Compare February 18, 2018 09:09

Vittly merged commit fa5fdf0 into master Feb 18, 2018

Vittly deleted the init branch February 18, 2018 09:12

Add code #1

Add code #1

Conversation

Vittly commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qfox Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qfox Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qfox Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qfox Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vittly Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vittly commented Feb 4, 2018

Vittly commented Feb 9, 2018

Vittly commented Feb 18, 2018

Vittly commented Jan 31, 2018 •

edited

Loading

qfox Feb 1, 2018 •

edited

Loading

qfox Feb 13, 2018 •

edited

Loading

qfox Feb 1, 2018 •

edited

Loading

qfox Feb 1, 2018 •

edited

Loading

Vittly Feb 4, 2018 •

edited

Loading