#9 API changes for File and Utils modules #10

tormozz48 · 2015-08-31T16:01:53Z

blond · 2015-09-01T06:24:00Z

lib/file.js

+ * @param {Boolean} inlineSourceMap - set to true to add source map code to content
+ * @returns {string}
+ */
+ render: function (inlineSourceMap) {


Лучше сделать объек опций options, с полем includeComment.

includeComment: 'inline' | true — добавит однострочный коментарий // ....

includeComment: 'block' — добавит многострочный коментарий /* ... */.

И если вызвать file.render() без аргументов, то возвращается { contents, map } и в contents не добавляется коментарий.

tormozz48 · 2015-09-01T08:36:23Z

🆙

blond · 2015-09-01T09:35:00Z

tests/file.test.js

@@ -16,7 +17,8 @@ describe('File', function () {
 it('should add a new line to the output', function () {
 file.writeLine('line 1');
 file.writeLine('line 2');
- file.render().should.equal('line 1\nline 2\n');
+ file.render().contents.should.equal('line 1\nline 2\n');
+ expect(file.render().map).to.be.equal(null);


Зачем проверять то, что поле с картой пустое?

tormozz48 · 2015-09-01T13:32:33Z

🆙

blond · 2015-09-02T14:29:08Z

lib/file.js

 var content = this._content.join(os.EOL);
+ var sourceMap = null;


Кажется, что это не совсем ок, потому что null и undefined это не одно и тоже.

Если кто-то будет проверять через typeof, то у него не получится.

typeof res.map === 'undefined'

Я бы оставил просто var sourceMap;.

tormozz48 · 2015-09-03T14:29:31Z

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

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

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

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

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

Ошибся в jsdoc

tormozz48 · 2015-09-03T14:29:52Z

🆙

j0tunn · 2015-09-03T21:30:06Z

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

j0tunn · 2015-09-03T21:31:07Z

lib/file.js

 }
- return content;
+ return {
+ contents: contents,


а почему, действительно, contents, а не content?

blond · 2015-09-04T08:29:52Z

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

tormozz48 · 2015-09-04T09:38:01Z

🆙

tormozz48 · 2015-09-04T09:41:04Z

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

Это консистентно с методом getCursor
ИМХО реализация геттеров и сеттеров в js оставляет желать лучшего так как по коду foo.bar
непонятно, что это: геттер или поле bar объекта foo

j0tunn · 2015-09-04T09:45:27Z

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

tormozz48 · 2015-09-04T09:48:42Z

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

j0tunn · 2015-09-04T09:51:01Z

lib/file.js

+ getContent: function () {
+ return this._content.join(os.EOL);
+ },
+
 render: function () {
 var content = this._content.join(os.EOL);


вот здесь вполне можно заюзать getContent

j0tunn · 2015-09-04T10:06:05Z

tests/file.test.js


- file.render();
+ file.render({ includeComment: 'block' });


забыл убрать

j0tunn · 2015-09-04T10:07:37Z

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

tormozz48 · 2015-09-04T11:29:14Z

🆙

j0tunn · 2015-09-04T11:32:04Z

lib/file.js

- }
- return content;
+ return this._map ?
+ utils.joinContentAndSourceMap(this.getContent(), this._map, this._opts) : this.getContent();


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

return this._map ? utils.joinCon... : this.getContent();

tormozz48 · 2015-09-04T12:07:02Z

🆙

j0tunn · 2015-09-04T12:13:50Z

tests/file.test.js

@@ -164,7 +162,7 @@ describe('File', function () {

 file.writeFileContent('some-file.js', middleContent);

- var pos = utils.getSourceMap(file.render()).originalPositionFor({line: 1, column: 0});
+ var pos = getOriginalSourceMapPosition(file.getSourceMap());


блин, андрюха, ты издеваешься? ) саму позицию-то ты зачем унес в хелпер?
Должно быть как-то так:

var pos = getOriginalPositionFor({line: 1, column: 0}, file.getSourceMap());

Нет, не издеваюсь

Во всех случаях использования были именно такие аргументы которые указывают на начальную позицию.

Здесь лишь разумный компромисс между гибкостью использования хелпера и размером копипаста

j0tunn · 2015-09-04T12:14:27Z

в остальном 🆗

tormozz48 · 2015-09-04T12:42:12Z

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

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

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

tormozz48 · 2015-09-04T14:29:05Z

Так, стоп.

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

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

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

tormozz48 · 2015-09-04T14:42:04Z

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

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

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

j0tunn · 2015-09-04T15:40:51Z

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

tormozz48 · 2015-09-04T15:47:39Z

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

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

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

blond · 2015-09-04T17:54:39Z

👍

blond · 2015-09-04T17:55:08Z

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

j0tunn · 2015-09-04T20:57:45Z

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

tormozz48 · 2015-09-07T07:39:59Z

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

#12

…nd sourceMap

…nerator wrapper)

#9 API changes for File and Utils modules

tormozz48 added the review label Aug 31, 2015

blond reviewed Sep 1, 2015
View reviewed changes

blond reviewed Sep 2, 2015
View reviewed changes

j0tunn reviewed Sep 3, 2015
View reviewed changes

j0tunn reviewed Sep 4, 2015
View reviewed changes

tests/file.test.js

file.render();

file.render({ includeComment: 'block' });

Copy link

Contributor

j0tunn Sep 4, 2015

Choose a reason for hiding this comment

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

забыл убрать

j0tunn reviewed Sep 4, 2015
View reviewed changes

tormozz48 force-pushed the issue-9 branch from b77202b to 002071a Compare September 4, 2015 12:37

tormozz48 added 2 commits September 8, 2015 10:10

#9 API changes for File module. Add get methods for retrive content a…

96bf82d

…nd sourceMap

#9 Add ability to take sourceMap as simple object (without SouceMapGe…

e8dff3a

…nerator wrapper)

tormozz48 force-pushed the issue-9 branch from 999026d to e8dff3a Compare September 8, 2015 07:11

tormozz48 added a commit that referenced this pull request Sep 8, 2015

Merge pull request #10 from enb-make/issue-9

55581dc

#9 API changes for File and Utils modules

tormozz48 merged commit 55581dc into master Sep 8, 2015

tormozz48 removed the review label Sep 8, 2015

tormozz48 deleted the issue-9 branch September 8, 2015 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9 API changes for File and Utils modules #10

#9 API changes for File and Utils modules #10

tormozz48 commented Aug 31, 2015

blond Sep 1, 2015

blond Sep 1, 2015

tormozz48 commented Sep 1, 2015

blond Sep 1, 2015

tormozz48 commented Sep 1, 2015

blond Sep 2, 2015

tormozz48 commented Sep 3, 2015

tormozz48 commented Sep 3, 2015

j0tunn commented Sep 3, 2015

j0tunn Sep 3, 2015

blond commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn Sep 4, 2015

j0tunn Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn Sep 4, 2015

tormozz48 Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

blond commented Sep 4, 2015

blond commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 7, 2015

		var content = this._content.join(os.EOL);
		var sourceMap = null;

#9 API changes for File and Utils modules #10

#9 API changes for File and Utils modules #10

Conversation

tormozz48 commented Aug 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tormozz48 commented Sep 1, 2015

Choose a reason for hiding this comment

tormozz48 commented Sep 1, 2015

Choose a reason for hiding this comment

tormozz48 commented Sep 3, 2015

tormozz48 commented Sep 3, 2015

j0tunn commented Sep 3, 2015

Choose a reason for hiding this comment

blond commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

Choose a reason for hiding this comment

tormozz48 commented Sep 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 4, 2015

blond commented Sep 4, 2015

blond commented Sep 4, 2015

j0tunn commented Sep 4, 2015

tormozz48 commented Sep 7, 2015