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

Add an option to wrap merged files to IIFE #27

Merged
merged 1 commit into from
Jul 10, 2015
Merged

Add an option to wrap merged files to IIFE #27

merged 1 commit into from
Jul 10, 2015

Conversation

gela-d
Copy link
Contributor

@gela-d gela-d commented Jul 8, 2015

Close #6

@gela-d gela-d added the review label Jul 8, 2015
@gela-d gela-d changed the title Add iife-option for joined files Add and option to wrap merged files to IIFE Jul 9, 2015

return [
'/* begin: ' + fn + ' */\n',
iife && 'function(){\n',
Copy link
Member

Choose a reason for hiding this comment

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

Это не надёжная запись, если убрать переносы строк код не сработает.

Возможные варианты: http://stackoverflow.com/questions/8228281/what-is-the-function-construct-in-javascript.

Я бы просто обернул в круглые скобки:

(function(){

}());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Исправлю.

@blond
Copy link
Member

blond commented Jul 9, 2015

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

@gela-d
Copy link
Contributor Author

gela-d commented Jul 9, 2015

Замечания исправила (кроме toString()), про изолированность сейчас подумаю.

@gela-d
Copy link
Contributor Author

gela-d commented Jul 9, 2015

@blond Добавила тест про изолированность 6c78763

iife = this._iife || '';

return [
'/* begin: ' + relPath + ' */\n',
Copy link
Member

Choose a reason for hiding this comment

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

Использовать \n не правильно для Windows.

Нужно использовать require('os').EOL.

@blond
Copy link
Member

blond commented Jul 10, 2015

Соскваш коммиты, в остальном 🆗

@gela-d gela-d changed the title Add and option to wrap merged files to IIFE Add an option to wrap merged files to IIFE Jul 10, 2015
@gela-d
Copy link
Contributor Author

gela-d commented Jul 10, 2015

@blond готово, спасибо!

blond added a commit that referenced this pull request Jul 10, 2015
Add an option to wrap merged files to IIFE
@blond blond merged commit c7fe821 into master Jul 10, 2015
@blond blond removed the review label Jul 10, 2015
@blond blond deleted the issues-6 branch July 10, 2015 14:48
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.

3 participants