enb / enb-source-map

ENB Source Maps Helper
MIT License
7 stars 2 forks source link

#9 API changes for File and Utils modules #10

Closed tormozz48 closed 9 years ago

tormozz48 commented 9 years ago

Resolved #9 Needs for https://github.com/enb-make/enb-diverse-js/issues/12

@j0tunn @blond Please review it

tormozz48 commented 9 years ago

:up:

tormozz48 commented 9 years ago

:up:

tormozz48 commented 9 years ago

и, кстати, как-то очень странно хотеть в конструкторе указывать один тип комментов, а името возможность это переопределить

Да, вопрос хороший. @blond что скажешь?

Или нужно именно иметь и склеенную строку и сам sourcemap отдельно?

Кажется, что прикольно иметь единый возвращаемый тип данных

а зачем несколько строк? У нас в конструктор передается опция comment и там inline - это значит использовать //, а block - /* */

Ошибся в jsdoc

tormozz48 commented 9 years ago

:up:

j0tunn commented 9 years ago

Есть такое предложение: не добавлять функциональности в render, пусть он как и раньше генерит склеенный контент. Можно добавить в File два геттера: content, sourcemap. нужен отдельно контент - file.content нужен отдельно sourcemap - file.sourcemap нужен склеенный кусок - file.render()

blond commented 9 years ago

C Антоном согласен.

tormozz48 commented 9 years ago

:up:

tormozz48 commented 9 years ago

Сделал не геттерами в их строгом понимании а псевдогеттерами потому что:

j0tunn commented 9 years ago

getCursor вообще должен быть приватным. А пользователю класса должно быть пофигу что это, по сути это и есть поле bar объекта foo, только в самом объекте оно хранится несколько в другом виде, а перед запросом преобразовывается. С геттерами есть проблемы в другом: при тестировании их не застабаешь, и при наследовании есть проблемы с их переопределением (особенно в inherit)

tormozz48 commented 9 years ago

Ок, мне переделать или оставить как есть?

j0tunn commented 9 years ago

нужно почистить мусор от предыдущей реализации и поправить тесты: везде, где раньше использовался render как способ проверки использовать соответствующие геттеры

tormozz48 commented 9 years ago

:up:

tormozz48 commented 9 years ago

:up:

j0tunn commented 9 years ago

в остальном :ok:

tormozz48 commented 9 years ago

Поправил последнее замечание Объединил коммиты

P.S. кстати, несмотря на наличие travis.yml конфигурационного файла, что-то я не вижу интеграции и проверки тревисом тестов. Наверное включить CI для данного репозитория.

P.S. Нужно будет выпустить обновленную версию enb-source-map

tormozz48 commented 9 years ago

Так, стоп.

Я тут попробовал использовать изменения которые мы внесли и обнаружил, что не все ок. В enb-diverse-js мне нужно вызывать метод joinContentAndSourceMap модуля Util и передавать в него минифицированный код и объект соурсмапа который отдает минификатор.

Но joinContentAndSourceMap сейчас принимает только экземпляры класса SourceMapGenerator, и это плохо

Нужно чтобы joinContentAndSourceMap умел работать и с экземплярами класса SourceMapGenerator и с обычными объектами sourceMap

tormozz48 commented 9 years ago

Нужно чтобы joinContentAndSourceMap умел работать и с экземплярами класса SourceMapGenerator и с обычными объектами sourceMap

Реализовал в последнем коммите.

Проверил для enb-diverse-js. Все ок.

j0tunn commented 9 years ago

я ж тебя об этом спрашивал :ok:

tormozz48 commented 9 years ago

я ж тебя об этом спрашивал

Ну лучше поздно, чем никогда.

P.S. Наверное можно мержить и выпускать версию

blond commented 9 years ago

:+1:

blond commented 9 years ago

А давайте перед выпуском обновим зависимости? А то source-map сейчас очень старый.

j0tunn commented 9 years ago

а давайте зависимости отдельным PR

tormozz48 commented 9 years ago

а давайте зависимости отдельным PR

12