Do not build graph by default #357

j0tunn · 2015-09-11T20:43:13Z

There are two changes:

by default make platform will not generate build graph (breaking change)
separate class for node with graph. To avoid lot of if (this._graph) checks

j0tunn · 2015-09-11T21:02:30Z

@blond @SwinX
Собственно зачем это нужно:

во-первых граф жрет достаточно много памяти: на сборке gemini-бандлов в islands это где-то 0.5 Mb, на сборке desktop.examples в islands - около 10 Mb. Ну и в большинстве случаев он никуда не показывается, так что строится впустую
во-вторых время сборки. Тут прирост небольшой, но тоже есть. При общем времени сборки desktop.examples в 7 минут 50 секунд
- если вообще граф не строить, то приблизительно -30-35 секунд (около 6-7%)
- если собирать с графом с NodeWithGraph, то около -10-15 секунд (около 2-3%)

blond · 2015-09-12T19:52:27Z

lib/test/mocks/test-node.js

@@ -64,9 +64,6 @@ module.exports = inherit({
    setTargetsToClean: function () {
        throw new Error('Method `setTargetsToClean` is not implemented.');
    },
-    setBuildGraph: function () {


Наверное, надо поправить это и в mock-enb.

А mock-enb разве не собираются закопать? Или я что-то путаю?

Пока на нем написана куча тестов - нет. В перспективе - возможно да

blond · 2015-09-12T19:54:37Z

@j0tunn крутяк! 💥

blond · 2015-09-12T19:58:50Z

Понимаю, что перенос node в папку выглядит логичным. Но предлагаю оставить файл lib/node.js на том же месте чтобы никому ничего не сломать.

Можно добавить ноду без графа в файл lib/node-without-graph, чтобы совсем не изменилась логика.

Или lib/node/index.js сделать просто прокидывателем lib/node/node-with-graph.js.

j0tunn · 2015-09-12T20:00:46Z

чтобы никому ничего не сломать

что именно я этим кому-то могу сломать?
Ну и тут в принципе ломающее изменение - граф по-умолчанию не строится

blond · 2015-09-12T20:01:44Z

lib/node/index.js

+var Node = require('./node');
+var NodeWithGraph = require('./node-with-graph');
+
+exports.mk = function (nodePath, makePlatform, cache, graph) {


Выглядит совсем не как публичное API.

всмысле? Это просто фабричный метод

Мне не нравится, что добавляется новое API, которое выглядит как публичное, но публичным не является.

Да и особой пользы от такого метода не вижу.

Это не публичное API, и я не пойму что значит "выглядит как публичное"

Да и особой пользы от такого метода не вижу.

Это паттерн проектирования "Фабричный метод", он позволяет инкапсулировать логику по выбору конкретной реализации, и при этом предоставляет общий интерфейс для работы с объектом. Пользователю этого метода пофигу какой конкретно объект будет создан, главное, что у него будет нужный API. Он вообще может не знать, что там разные объекты для разных аргументов создаются.
В данном конкретном случае я унес логику по выбору объекта с графом/без графа из makePlatform.

@j0tunn Если это единственная функция в модуле, может её экспортировать как module.exports = function() {};?

blond · 2015-09-12T20:15:57Z

что именно я этим кому-то могу сломать?

Если рекваерить класс Node, то после изменения приедет lib/node/index.js, а не то, что ожидалось.

Ну и тут в принципе ломающее изменение - граф по-умолчанию не строится

Можно сделать это изменение не ломающим. Если вызывать через CLI — при опции --graph нужно чтобы граф строился, без указания чтобы не строился.

Через API точно так же, если передать опцию graph: true то строится, если не передать, то не строится.

А если подключать Node класс, то чтобы приезжал прежний класс с графом.

j0tunn · 2015-09-12T20:20:35Z

Андрюх, зачем все эти сложности с обратной совместимостью? Ты имеешь ввиду какие-то конкретные пакеты, которые реквайрят Node?

blond · 2015-09-12T20:36:31Z

Пользователи ENB привыкли к тому, что ничего не ломается от версии к версии.

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

До версии 0.15.0 у ENB вообще не было JS API и единственный возможный способ был подключать lib/make.js, создавать новый инстанс и работать с ним. Я лично очень много где используют lib/make.js таким образом.

Кроме того, для написания своих технологий считается нормально подключать что угодно из lib и использовать.

Поэтому нужно считать всё что лежит в lib и techs как публичное API.

j0tunn · 2015-09-12T21:04:01Z

я не нашел ни одного репозитория, где бы подключалась node из enb. Пролистал все 334 результата по запросу require('enb/lib/node') language:JavaScript - ноль.
Если ты имеешь ввиду какие-то конкретные пакеты - пиши, я сделаю в них PR.
Я планирую вносить достаточно много изменений в enb, и обратная совместимость все равно будет ломаться.

просто будет чуть менее красиво, и кажется, тут легко можно этим пожертвовать

ты не путай теплое с мягким, мне пофиг насколько красиво выглядит код, мне важно качество, а качеством кода я жертвовать не собираюсь

Пользователи ENB привыкли к тому, что ничего не ломается от версии к версии.

придется отвыкать, иначе не получится сделать конфетку, enb в ближайшем будущем ждет большое количество изменений, которые так или иначе будут затрагивать основные части ядра, и если для каждого из этих изменений мы будем жертвовать качеством в угоду каким-то абстрактным пользователям (которые с какого-то бодуна полезли во внутренности ядра), то мы попросту присыпем воняющую кучу сахарной пудрой.
Есть фиксированное API - пользуйтесь им, нужно что-то изнутри - делайте PR чтобы вынести это наружу. Все внутренности - для внутреннего использования

blond · 2015-09-12T21:27:51Z

Почему не делать много изменений, ломающих обратную совместимость в рамках 1.x?

arikon · 2015-09-13T08:49:35Z

До версии 0.15.0 у ENB вообще не было JS API и единственный возможный способ был подключать lib/make.js, создавать новый инстанс и работать с ним. Я лично очень много где используют lib/make.js таким образом.

Как выглядит публичное API начиная с версии 0.15.0? Затрагивают ли изменения Антона это API обратно несовместимым образом?

arikon · 2015-09-13T08:53:30Z

lib/make.js

@@ -325,9 +332,9 @@ module.exports = inherit( /** @lends MakePlatform.prototype */ {
            var _this = this;
            var cdir = this.getDir();
            var nodeConfig = this._projectConfig.getNodeConfig(nodePath);
-            var node = new Node(nodePath, this, this._cache);
+            var node = Node.mk(nodePath, this, this._cache, this._graph);


var createNode = require('lib/node'); // ... var node = createNode(nodePath, this, this._cache, this._graph);

Ну или nodeFactory вместо createNode.

М?

когда модуль сразу экспортит функцию - это сложнее тестировать (не получится застабать, нужно плясать с бубном и чем-то типа proxyquire)

SwinX · 2015-09-14T10:49:53Z

Отписал замечания.

j0tunn · 2015-09-14T14:50:36Z

Для адекватного тестирования NodeWithGraph нужна вот эта фича: dfilatov/inherit#21
Иначе придется писать:

Node.prototype.resolveTarget.apply(this, arguments);

вместо простого вызова this.__base

j0tunn · 2015-09-14T15:49:45Z

Пока переписал на Node.prototype...

j0tunn · 2015-09-21T08:42:36Z

Предлагаю вливать, а в inherit мы будем думать как лучше реализовать

j0tunn · 2015-09-23T12:17:18Z

отрибейзился

SwinX · 2015-09-23T12:25:19Z

👍

Do not build graph by default

levonet added the review label Sep 11, 2015

j0tunn force-pushed the feature/graph.on.demand branch from 495a954 to ca744e5 Compare September 11, 2015 20:54

blond reviewed Sep 12, 2015
View reviewed changes

arikon reviewed Sep 13, 2015
View reviewed changes

j0tunn force-pushed the feature/graph.on.demand branch 2 times, most recently from 1e1a69f to 9cc6277 Compare September 17, 2015 19:45

j0tunn force-pushed the feature/graph.on.demand branch from 9cc6277 to 132a6cb Compare September 17, 2015 19:47

j0tunn added 3 commits September 23, 2015 15:13

make: create build graph only when it needed

d052842

Separate class NodeWithGraph for building graph

5f97701

codestyle

bbb78f3

j0tunn force-pushed the feature/graph.on.demand branch from 132a6cb to bbb78f3 Compare September 23, 2015 12:13

SwinX added a commit that referenced this pull request Sep 23, 2015

Merge pull request #357 from enb-make/feature/graph.on.demand

b3f8292

Do not build graph by default

SwinX merged commit b3f8292 into master Sep 23, 2015

SwinX removed the review label Sep 23, 2015

SwinX deleted the feature/graph.on.demand branch September 23, 2015 15:00

j0tunn mentioned this pull request Sep 29, 2015

Added changelog for v1.0.0. Updated package version to 1.0.0 #403

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not build graph by default #357

Do not build graph by default #357

j0tunn commented Sep 11, 2015

j0tunn commented Sep 11, 2015

blond Sep 12, 2015

SwinX Sep 14, 2015

j0tunn Sep 14, 2015

blond commented Sep 12, 2015

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

blond Sep 12, 2015

j0tunn Sep 12, 2015

blond Sep 12, 2015

j0tunn Sep 12, 2015

arikon Sep 13, 2015

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

blond commented Sep 12, 2015

arikon commented Sep 13, 2015

arikon Sep 13, 2015

j0tunn Sep 13, 2015

SwinX commented Sep 14, 2015

j0tunn commented Sep 14, 2015

j0tunn commented Sep 14, 2015

j0tunn commented Sep 21, 2015

j0tunn commented Sep 23, 2015

SwinX commented Sep 23, 2015

Do not build graph by default #357

Do not build graph by default #357

Conversation

j0tunn commented Sep 11, 2015

j0tunn commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blond commented Sep 12, 2015

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

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

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

blond commented Sep 12, 2015

j0tunn commented Sep 12, 2015

blond commented Sep 12, 2015

arikon commented Sep 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwinX commented Sep 14, 2015

j0tunn commented Sep 14, 2015

j0tunn commented Sep 14, 2015

j0tunn commented Sep 21, 2015

j0tunn commented Sep 23, 2015

SwinX commented Sep 23, 2015