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.

Sign up for GitHub

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

Jump to bottom

Added config file support #32

Merged
merged 5 commits into from Nov 7, 2017
Merged

Added config file support #32

merged 5 commits into from Nov 7, 2017

Conversation

vbrvk
Copy link
Contributor

@vbrvk vbrvk commented Nov 5, 2017

Do I need to write more tests?

@vbrvk vbrvk changed the title #28 Added config file support Added config file support Nov 5, 2017
@ai
Copy link
Owner

ai commented Nov 5, 2017 •
edited

Thanks! Few ways to improve PR:

  1. Do we need readPkg? cosmiconfig can load config from package.json too.
  2. Users don’t like to have a choice. Maybe it is better to tell about .size-limit config in separated section (or Configs section). So in Usage section, we will still be very focused on the most popular solutions.
  3. Note, that new tests falls on Travis CI.

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 5, 2017

As I understand, with cosmiconfig we can't handle both names at package.json 'size-limit' and 'sizeLimit'.

@ai
Copy link
Owner

ai commented Nov 5, 2017

I am OK with removing legacy sizeLimit. Anyway, we showed a warning during few minor versions. Also because we are in 0.x versions, we can break public API in minor versions ;).

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 5, 2017

Should I check existing of package.json?

@ai
Copy link
Owner

ai commented Nov 5, 2017

If we have .size-limit, we don’t need package.json.

@ai
Copy link
Owner

ai commented Nov 5, 2017 •
edited

  1. If we want good docs, we should always think that user didn’t read whole docs. Right now Config section tells about only .size-limit (user may miss Usage section and don't read accurately). So I think it will be better to explicit say that there is a 2 ways. And show both ways together.
  2. We need to update error messages as well.
  3. Test still falls.

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 5, 2017

Tests are failed cause there is spelling checking, but I didn't change thet part of README

@ai
Copy link
Owner

ai commented Nov 5, 2017

Ouh. It is Yaspeller problem. I will fix it.

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017 •
edited

Appveyor test failed on index.test.js, I have no idea about this trouble, I didn't touch this stuff)

I will fix docs and error messages tomorrow

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017

About error messages, what do I need to fix? Can I add a link to the Config section at GitHub?

@ai
Copy link
Owner

ai commented Nov 6, 2017

Check out origin error messages. You will see that error messages were clear and tell exactly what and where should be changed.

settings for `"size-limit"

Why "size-limit" in quotes?

Add it according to Size Limit docs

Add where?

Can not parse "size-limit" config

What config and what it is exacty a error?

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017 •
edited

Why "size-limit" in quotes?

Because it was so before

@ai
Copy link
Owner

ai commented Nov 6, 2017

Because it was so before

Before it was about package.json section. This is a reason why it was in quotes. In your case, you don’t need to use quotes. Size Limit settings" is correct.

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017

you don’t need to use quotes

Ok, I will fix it.

Do you know what may be wrong with AppVeyor CI, how can I fix it?

@ai
Copy link
Owner

ai commented Nov 6, 2017 •
edited

Note, that "size-limit" quotes is not the only problem in error messages. I mentioned problems here #32 (comment)

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017

I have fixed error messages, now the parsing error contains the path to the config and explanation of the error.

ai
ai reviewed Nov 6, 2017
View reviewed changes
README.md Outdated
@@ -125,6 +125,35 @@ Add the `size` script to your test suite:
If you don't have a continuous integration service running, don’t forget
to add one — start with [Travis CI](https://github.com/dwyl/learn-travis).

## Config
Add `size-limit` section to `package.json` and `size` script:
Copy link
Owner

Choose a reason for hiding this comment

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

No newline after the title.

It is better to explicit say about 2 types of config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it good enough?

image

Copy link
Owner

Choose a reason for hiding this comment

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

Yeap, looks good.

ai
ai reviewed Nov 6, 2017
View reviewed changes
cli.js Outdated
}))
return limits
}).then(limits => {
return readPkg().then(packageJson => {
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need readPkg here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because below we have

return {
            webpack: limit.webpack !== false,
            bundle: packageJson.pkg.name,
            config: limit.config,
            ignore: packageJson.pkg.peerDependencies,
            limit: limit.limit,
            full: files.map(i => path.join(cwd, i)),
            files
          }

Copy link
Owner

Choose a reason for hiding this comment

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

Ouh :(.

ai
ai reviewed Nov 6, 2017
View reviewed changes
cli.js Outdated
}).then(limits => {
return readPkg().then(packageJson => {
return Promise.all(limits.map(limit => {
const cwd = path.dirname(packageJson.path)
Copy link
Owner

Choose a reason for hiding this comment

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

For .size-limit config you need to use its directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it

ai
ai reviewed Nov 6, 2017
View reviewed changes
test/cli.test.js Outdated
return run([], { cwd: fixture('wrong-file-config') }).then(result => {
expect(result.out).toContain(
' ERROR Can not parse Size Limit config: \n' +
'missed comma between flow collection entries'
Copy link
Owner

Choose a reason for hiding this comment

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

All other error messages have indent on next lines.

 ERROR  Can not parse Size Limit config:
        missed comma between flow collection entries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it

@ai
Copy link
Owner

ai commented Nov 6, 2017

Awesome. Thanks. I will accept and release it today evening.

@vbrvk
Copy link
Contributor Author

vbrvk commented Nov 6, 2017

Thanks for your tips!

@ai ai merged commit 5b9630e into ai:master Nov 7, 2017
@ai ai mentioned this pull request Nov 7, 2017
Closed
@ai
Copy link
Owner

ai commented Nov 7, 2017

Released in 0.13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ai ai ai left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vbrvk @ai