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

Feature/add custom dir for output bundle #98

Merged
merged 8 commits into from Apr 28, 2019

Conversation

Projects
None yet
2 participants
@isolovev
Copy link
Contributor

commented Apr 28, 2019

No description provided.

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

  1. Let's name getSize option as output.
  2. You need to add CLI and getSize tests.
index.js Outdated
@@ -179,6 +187,7 @@ function getLoadingTime (size) {
* @param {boolean} [opts.gzip=true] Compress files by gzip.
* @param {string} [opts.config] A path to custom webpack config.
* @param {string} [opts.bundle] Bundle name for Analyzer mode.
* @param {string} [opts.directory] A path for output bundle.

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

@ai это не покрывает 1-ый пункт?

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Ага, только переименуй directory в output

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@ai абсолютные пути нужно обрабатывать?

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

абсолютные пути нужно обрабатывать?

Что ты имеешь в виду? Система должна работать как с относительными путями так и с абсолютными (но вроде спец. кода не нужно).

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Проверка является ли путь абсолютным, если да, то по нему положить, если нет, то с конкатинировать с process.cwd()
Добавил

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Подскажи, как тестировать данный параметр?
Проверять записан ли файл, а после его удалять?

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

Проверять записан ли файл, а после его удалять?

Ага. Пиши внутри os.tmpdir

index.js Outdated
@@ -29,6 +29,18 @@ function projectName (opts, files) {
}
}

function getPathBundle (opts) {

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

get path bundle — не корректная фраза на английском. getBundlePath

@@ -29,6 +29,18 @@ function projectName (opts, files) {
}
}

function getPathBundle (opts) {
if (opts.output) {
if (path.isAbsolute(opts.output)) {

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

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

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Проверил, вебпак выбрасывает ошибку

isolovev added some commits Apr 28, 2019

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Добавил тест в  cli.test.js
Подскажи, как лучше написать тест для index.test.js?

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

Подскажи, как лучше написать тест для index.test.js?

Так же. Вызываешь getSize(), проверяешь создался ли файл. Удаляешь его в конце.

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Ага. Добавил.

output: testDir
})

expect(fs.existsSync(join(testDir, 'index.js'))).toEqual(true)

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Ты забыл удалить файл

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Error: Cannot delete files/folders outside the current working directory. Can be overridden with the `force` option.
MacOS не дает это сделать. Думаю linux-like так же будет

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

А почему нельзя передать в del force аргумент, как указано в ошибке?

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Или force добавить?

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Ну да del(dir, { force: true })

@@ -56,6 +58,7 @@ afterAll(async () => {
await del(join(__dirname, 'fixtures/webpack-multipe-entry-points/dist'))
await del(join(__dirname, 'fixtures/webpack-config/dist'))
await del(join(__dirname, '../dist'))
await del(join(__dirname, 'build'))

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Это надо в after вместо afterAll (лучше перенести все в after.

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Не могу найти такой метод :(

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

На строчке 57 переименуй afterAll в after. Сейчас файлы удаляются в конце всех тестов. В итоге save output bundle to absolute path влияет на save output bundle to relative path (второй может не работать, но так как файл останется после первого, то тест пройдёт). after же вызывает функцию после каждого теста, гарантируя чистую среду.

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Может удаление в тест занести?

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Если тест не пройдёт, то файл не удалиться

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Добавил в сам тест и в afterAll. Ибо увеличиваем время afterEach с удаление файла, которого еще нет

{ cwd: fixture('defaults') }
)

expect(fs.existsSync(join(testDir, 'index.js'))).toEqual(true)

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

/tmp/index.js — это слишком простое имя. В ОС кто-то другой мог такой файл создать. Давай хоть join(testDir, 'size-limit-bundle.js')

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Я уже добавил имя директории, чтобы ее удалять а не 2 файла в отдельности. Ибо флаг "без gzip" не передаю

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

os.tmpdir() возвращает просто /tmp (то есть глобальную папку, она не создаёт какую-то специальную)

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Добавил папку и проверяю ее, а не файл


it('save output bundle to relative path', async () => {
let { code } = await run(
['--save-bundle', '../../build'],

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Тут тоже лучше создавать файл в join(os.tmpdir(), 'size-limit-cli-bundle.js') а не записывать в репо

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Тогда смысл этого теста, если проверяем абсолютный путь.
После каждого теста папка затирается. И можно в gitignore занести

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

А, точно. Забыл что это относительный пути

This comment has been minimized.

Copy link
@ai

ai Apr 28, 2019

Owner

Да, папку надо в .gitignore (и назвать более понятно типа output-test), чтобы если тесты упали ддо вызыва after()

This comment has been minimized.

Copy link
@isolovev

isolovev Apr 28, 2019

Author Contributor

Добавил в .gitignore

isolovev added some commits Apr 28, 2019

@isolovev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Еще что-то нужно добавлять/править?

@ai ai merged commit 0a7ae6e into ai:master Apr 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ai

This comment has been minimized.

Copy link
Owner

commented Apr 28, 2019

Released in 1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.