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

Make level configuration more abstract. Closes #319 #320

Closed
wants to merge 1 commit into from

Conversation

tadatuta
Copy link
Member

@tadatuta tadatuta commented Mar 7, 2014


return levels
return (this.getLevelsByBundleName()[buildLevel] || [])
Copy link
Member

Choose a reason for hiding this comment

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

Если уж getLevelsByBundleName, то как минимум, bundleName (а не bundleLevel) должен в параметр метода передаваться.

Copy link
Member Author

Choose a reason for hiding this comment

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

это ж не параметр метода, а обращение по ключу к объекту, который из getLevelsByBundleName() возвращается

Copy link
Member

Choose a reason for hiding this comment

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

Я выше решил «схалявить и упаковал» в предыдущем комменте несколько замечаний» в одно высказывания — не прокатило ;)

1.1) Метод называется getLevelsByBundleName — это не правда. Он возвращает не список уровней, а какой-то объект который потом надо еще обработать.
1.2) Тем более, возвращаемое «что-то» никак не соотносится с чтотоByЧтото — метод вообще не принимает агрументы. Откуда By?
2) Часть про BundleName никак не «коррелирует» ;) с тем, что внутри него все опирается на BundleLevel.
Т.е. pages, bundles или desktop-pages — это BundleLevelName; какой-нибудь index — это BundleName

@tadatuta
Copy link
Member Author

tadatuta commented Mar 8, 2014

@narqo Дошло, переименовал в getLevelsMap.

Кроме того обнаружил, что от бандлового getLevels наследуется SpecNode и ожидает, что там окажется именно bundleName. Поправил.

@narqo
Copy link
Member

narqo commented Mar 8, 2014

Может вообще все это унести в getLevels?

@tadatuta
Copy link
Member Author

tadatuta commented Mar 8, 2014

а чем это лучше? при внесении в getLevels получится ведь просто локальная переменная вместо метода, который потенциально можно переопределять. при этом сейчас getLevelsMap вообще без какой-либо логики и почти тянет на интуитивно понятную декларацию. если пойти чуть дальше и вынести такой же простой getTechs и getLevelsMap во внешний конфиг, то есть надежда, что большинству пользователей этого будет достаточно и не придется разбираться в хитростях конфигурирования сборки (на самом деле у меня тут есть более насущная цель — упростить код генератора стаба).

@narqo
Copy link
Member

narqo commented Mar 9, 2014

Так ведь никто не запрещает переопределять getLevels / getTechs? Ты же и сейчас можешь унести описание уровней во конфиг .bem/levels.js бандла. Просто есть большие сомнения про то, насколько это удобно.

getLevelsMap вообще без какой-либо логики и почти тянет на интуитивно понятную декларацию.

Вот я очень сильно против таких громких высказываний :) getLevels / getTechs тянет на что-то понятное и интуитивное.

Если в проекте есть один уровень бандлов — bundles. Я из getLevels просто возвращаю массив уровней, по аналогии с getTechs. Вроде просто и абстрактно. Если появится понятие «платформа», разбить getLevels на более мелкие платформо-специфичные методы, будет достаточно логичным — мы вроде все API так разрабатываем.

'design/touch.blocks',
'design/touch-phone.blocks'
];
getLevelsMap : function() {
Copy link
Member

Choose a reason for hiding this comment

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

вообще, в истории с хешом, мне больше всего не нравится, что если у тебя будет на каждую платформу примерно 20 уровней (как у нас в islands-*) хеш будет большой и необзорный.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ты же и сейчас можешь унести описание уровней во конфиг .bem/levels.js бандла.

проблема этого подхода в том, что конфигурация bem make оказывается разбросана по всему проекту. оно, конечно, полный БЭМ, все дела, но по факту начать в такой стуктуре разбираться пользователям сложно.

хочется ситуации, когда есть make.js, он может обладать неким магическим знанием, но при этом предоставлять такие хелперы, которые бы позволяли в 99% случаев не разбираться с кодом самого make.js и декларировать всю кастомизацию в конфиге, состоящем из массивов и хешей.
если снабдить такой конфиг jsdoc-ом, где про getTechs написать, что метод «возвращает список технологий, которые необходимо собрать» и аналогично для собираемых уровней, то, кмк, этим можно будет начать пользоваться без глубокого погружения. а генератору стабов в идеале будет достаточно делать JSON.stringify и писать результат в один файл, а не инжектиться в множесте файлов во множество мест.

но я согласен, что для проекта с одним уровнем это все лишнее. просто хочется, чтобы было консистентно, иначе задача генерации конфигов каждого проекта превращается в адовый хардкод.

хеш будет большой и необзорный

согласен, но вроде и текущий вариант тут не особо спасает :(

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.

None yet

2 participants