Proof of Concept: engines as plugins #371

awinogradov · 2016-10-09T15:26:32Z

awinogradov · 2016-10-09T15:51:51Z

lib/bemtree/index.js

@@ -6,7 +6,6 @@ function BEMTREE() {
  BEMXJST.apply(this, arguments);


We can move this to plugin constructor

miripiruni · 2016-10-10T13:12:31Z

Why we can’t just add get bemxjst() here?

awinogradov · 2016-10-10T13:58:41Z

@miripiruni we can) but fs. And it's not the same. The idea of this API is extending base prototype by methods from custom engines. Every next plugin gets a prev prototype and extends(overrides) its methods. It's more powerful way of usage I think.

miripiruni · 2016-10-11T13:17:36Z

The idea of this API is extending base prototype by methods from custom engines.

I don’t think we need extending more than once (like plugins composition). So can we just expose bemxjst as a require?

Like:

var Bemxjst = require('./lib/bemxjst');

var api = new Bemxjst({/* options */});
var templates = {};

api.compile(fn);
api.exportApply(templates);

return templates;

Then you can extend Bemxjst as you want and use with webpack and etc...

miripiruni

В целом я по-прежнему скептически настроен по поводу растаскивания движка bem-xjst на разные слои.

Во-первых, это затруднит тестирование.
Во-вторых, очевидно скажется на производительности.
В-третьих, замедлит разработку — нужно будет учитывать что всё разбирается по уровням которые собираются в произвольном порядке (xjst + bem + html или xjst + html или xjst + bemtree или etc) и я думаю это того не стоит.

Я предлагаю вытащить bemxjst в get bemxjst вот сюда https://github.com/bem/bem-xjst/blob/master/index.js#L19 и дать возможность переопределять Tree.

miripiruni · 2016-10-11T14:34:31Z

lib/bemxjst/index.js

@@ -9,6 +8,10 @@ var utils = require('./utils');

 function BEMXJST(options) {
  this.options = options || {};
+  this.options.Tree = this.options.Tree || require('./tree').Tree;


Зачем require здесь?

require('./tree').Tree оставим во второй строке как переменную. Здесь оставим

if (this.options.Tree) Tree = this.options.Tree;.

miripiruni · 2016-10-11T14:35:00Z

lib/bemxjst/index.js

@@ -9,6 +8,10 @@ var utils = require('./utils');

 function BEMXJST(options) {
  this.options = options || {};
+  this.options.Tree = this.options.Tree || require('./tree').Tree;
+  this.options.locals = this.options.Tree.methods.concat(


Соответственно здесь и в других местах ничего не изменится.

Наверное, не стоит экстендить то, что передаётся нам по ссылке.

miripiruni · 2016-10-11T14:38:17Z

example.js

+  );
+}, {fuck: 'off'});
+
+// TODO: separate bem-methodolgy as plugin too


Очень дикая идея. Это правда кому-то надо? На что матчится будем? Слишком много переписывать надобудет. Цепочка прототипов вырастет. Скорее всего будет медленно.

Покажу в другом PR, а может и в этом. Вай нот

miripiruni · 2016-10-11T14:38:56Z

package.json

    "chai": "^3.5.0",
+    "istanbul": "^0.4.4",


Ненужный дифф.

Это не я)

miripiruni · 2016-10-11T14:39:07Z

package.json

    "jscs": "^3.0.3",
    "jshint": "^2.7.0",
-    "mocha": "^3.0.2"
+    "mocha": "^3.0.2",
+    "xjst-ddsl": "github:awinogradov/xjst-ddsl#pluggable"


Это временно?

Да, там такие же изменения просто как и в bemhtml с bemtree для примеров

miripiruni · 2016-10-11T14:49:32Z

pluggable.js

+  constructor(templates = '', options = {}) {
+    this.options = options;
+    this.templates = [templates];
+    this.engine = require(`./lib/bemxjst`);


require в конструкторе не самая хорошая идея.

Вот это точно временно)

miripiruni · 2016-10-11T14:50:23Z

pluggable.js

+  }
+
+  use(plugin) {
+    console.log(`apply engine: ${plugin.name}`);


Это временно?

miripiruni · 2016-10-11T14:51:21Z

pluggable.js

+    this.engine = plugin.engine;
+
+    const options = Object.assign(this.options, plugin.options);
+    const instance = new this.engine(options);


Смущает, что на каждый вызов .use() создаётся новый экземпляр this.engine()

this.Engine() — с большой буквы, это же конструктор.

Не понятно почему смущает. Комбайним конструктор перед рендером шаблонов. Сейчас мы делаем тоже самое. По факту use будет принимать массив плигнов и делать и new один раз. В текущем виде можно вынести new в apply или что-то такое.

miripiruni · 2016-10-11T15:25:58Z

example.js

+// apply engine: bemtree
+// { block: 'b', content: 'shit' }
+
+xjst.use(bemhtml({ omg: 'bemhtml' }));


Что будет если я буду писать несколько раз подряд

xjst.use(bemhtml({ omg: 'bemhtml' })); xjst.use(bemtree({ omg: 'bemhtml' })); xjst.use(bemhtml({ omg: 'bemhtml' })); xjst.use(bemtree({ omg: 'bemhtml' }));

? Ведь ты предоставляешь такую возможность.

Но ведь ты сам виноват тогда.

Соглашусь с @zxqfox, ибо ты сам должен головой думать. С плагинами PostCSS таже история, никто не тестирует все возможные комбинации их порядка следования. Просто каждый должен выполнять одну законченную мысль. И да они многие из них конфликтуют из-за порядка. Это кажется нормально.

miripiruni · 2016-10-11T15:27:37Z

example.js

+// apply engine: bemhtml
+// <div class="b" id="the-id">fuckin shit</div>
+
+xjst.use(ddsl({ omg: 'ddsl' }));


Сейчас вся видимая красота конструкции use не имеет смысла, так как движки тесно связаны между собой и перекрывают методы друг друга в том порядка, в котором это предусмотрено. Написав неправильный порядок или убрав недостающий кирпич всё развалится.

По твоим кейсам этого не видно. Нужны полноценные примеры с покрытием всех вариантов.

miripiruni · 2016-10-11T15:35:41Z

pluggable.js

+
+    // member templates between engine changes
+    this.templates.forEach(templates => {
+      this.compile(templates);


При втором и последующих вызовах use() все шаблоны будут компилироваться заново, хотя это требует ощутимого времени и они уже компилировались в предыдущий раз.

Такая жизнь) Движок сменился, надо перекомпилить. Если есть идеи как лучше, то велкам

propose plugins

ab9100a

levonet added the status: review label Oct 9, 2016

awinogradov mentioned this pull request Oct 9, 2016

unopinionaited generic xjst runtime #287

Closed

awinogradov commented Oct 9, 2016

View reviewed changes

extandable tree

0a3a4c7

miripiruni requested changes Oct 11, 2016

View reviewed changes

veged changed the title ~~PoC: engines as plugins~~ Proof of Concept: engines as plugins Oct 12, 2016

miripiruni force-pushed the master branch from fe64f93 to 79b3e4e Compare December 2, 2016 12:36

miripiruni closed this Feb 1, 2017

miripiruni removed the status: review label Feb 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of Concept: engines as plugins #371

Proof of Concept: engines as plugins #371

awinogradov commented Oct 9, 2016

awinogradov Oct 9, 2016

miripiruni commented Oct 10, 2016

awinogradov commented Oct 10, 2016

miripiruni commented Oct 11, 2016 •

edited

Loading

miripiruni left a comment

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

miripiruni Oct 11, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016

qfox Oct 11, 2016

awinogradov Oct 12, 2016

miripiruni Oct 11, 2016 •

edited

Loading

miripiruni Oct 11, 2016

awinogradov Oct 12, 2016

		@@ -6,7 +6,6 @@ function BEMTREE() {
		BEMXJST.apply(this, arguments);

Proof of Concept: engines as plugins #371

Proof of Concept: engines as plugins #371

Conversation

awinogradov commented Oct 9, 2016

Choose a reason for hiding this comment

miripiruni commented Oct 10, 2016

awinogradov commented Oct 10, 2016

miripiruni commented Oct 11, 2016 • edited Loading

miripiruni left a 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

miripiruni Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miripiruni commented Oct 11, 2016 •

edited

Loading

miripiruni Oct 11, 2016 •

edited

Loading