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

Use requireNodeSources under the hood of dependantFiles #52

Merged
merged 1 commit into from
Nov 25, 2016

Conversation

tadatuta
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-35.3%) to 64.706% when pulling a91a7e9 on requireNodeSources into 0944d1e on master.

Copy link
Member

@blond blond left a comment

Choose a reason for hiding this comment

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

Нужно добавить документацию для нового API dependantFiles.


return jobQueue.push(modulePath, opts);
var nodePath = node.getPath();
Copy link
Member

Choose a reason for hiding this comment

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

Можно не дёргать метод в цикле

return jobQueue.push(modulePath, opts);
var nodePath = node.getPath();

acc[nodePath] || (acc[nodePath] = []);
Copy link
Member

Choose a reason for hiding this comment

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

Зачем? node не меняется.

var sources = {};
var currentNodeSources = sources[node.nodePath())] = [];

currentNodeSources.push(dependancy);

@@ -86,8 +86,22 @@ module.exports = buildFlow.create()
tech: this._tech,
techOptions: this._techOptions
},
jobQueue = this.node.getSharedResources().jobQueue;
jobQueue = this.node.getSharedResources().jobQueue,
sourcesByNodes = this._dependantFiles.reduce(function (acc, dependancy) {
Copy link
Member

Choose a reason for hiding this comment

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

Можно обойтись if на объект + map на конверт.

Choose a reason for hiding this comment

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

@tadatuta тут падает если передавать объект так как написано в доке

Copy link
Member Author

Choose a reason for hiding this comment

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

@voischev спасибо, #55

Copy link
Member

@blond blond left a comment

Choose a reason for hiding this comment

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

Сейчас можно указать только таргеты текущей ноды:

{ 
    dependantFiles: ['a.css', 'b.js']
}

PR добавляет возможность указать полную запись (для любой ноды):

{ 
    dependantFiles: {
        'path/to/node': ['a.css', 'b.js']
    }
}

При этом предыдущий вариант можно считать шорткатом к новому. Обратная совместимость не меняется. Тут всё хорошо.

Но смешанный вариант, мне не очень нравится:

{ 
    dependantFiles: [
        'a.css', 'b.js',
        {
            'path/to/node': ['a.css', 'b.js']
        }
    ]
}

Если нужно указать таргеты для нескольких нод, лучше явно указывать эти ноды:

{ 
    dependantFiles: {
        'path/to/node-1': ['a.css', 'b.js'],
        'path/to/node-2': ['a.css', 'b.js']
    }
}

@blond
Copy link
Member

blond commented Nov 9, 2016

@tadatuta есть причины поддерживать смешанный вариант?

@tadatuta
Copy link
Member Author

tadatuta commented Nov 9, 2016

Веских причин, конечно, нет, но поддержка ничего не стоит, а сахару добавляет.
Если ты жестко против — выпилю, конечно. Но как по мне — вполне норм возможность, а не хочешь — не пользуйся.

@tadatuta
Copy link
Member Author

м?

@blond
Copy link
Member

blond commented Nov 14, 2016

Давай в документации оставим только годный вариант, а извращенцы вроде тебя и сами догадаются.

В реализации, соответственно оставим.

@tadatuta
Copy link
Member Author

дописал в доку

@coveralls
Copy link

Coverage Status

Coverage decreased (-35.3%) to 64.706% when pulling b088b6a on requireNodeSources into 0944d1e on master.

Имена файлов, после сборки которых запустится обработка исходного файла с помощью `borschik`
Имена файлов, после сборки которых запустится обработка исходного файла с помощью `borschik`.
Если переданы строки, то ожидается, что это имена таргетов текущей ноды.
В случае передачи объекта, ключами является путь к ноде, а значениями — массив таргетов.
Copy link
Member

Choose a reason for hiding this comment

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

Надо примерами кода, а не буквами. Читается сложно.

@tadatuta
Copy link
Member Author

🆙

@coveralls
Copy link

Coverage Status

Coverage decreased (-35.3%) to 64.706% when pulling 8f37d58 on requireNodeSources into 0944d1e on master.

@tadatuta
Copy link
Member Author

ping?

Copy link
Member

@blond blond left a comment

Choose a reason for hiding this comment

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

И отребейзь от master, пожалуйста.

}
```

В случае передачи объекта, ключами является путь к ноде, а значениями — массив таргетов:
Copy link
Member

Choose a reason for hiding this comment

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

Чот как-то не по-русски.

И давай считать массив краткой записью. Тогда получится примерно так:

Опция ожидает объект, где ключ это путь к ноде, а значение — массив файлов из указанной ноды.

{
    dependantFiles: {
        'path/to/index': ['index.css', 'index.js'],
        'path/to/keyboard': ['keyboard.css', 'keyboard.js']
    }
}

Если переданы строки, то ожидается, что это имена таргетов текущей ноды:

{
    dependantFiles: ['index.css', 'index.js']
}

@coveralls
Copy link

Coverage Status

Coverage decreased (-35.3%) to 64.706% when pulling d355b8d on requireNodeSources into 606d3cf on master.

@blond blond merged commit a7ff43e into master Nov 25, 2016
@blond blond deleted the requireNodeSources branch November 25, 2016 09:29
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.

4 participants