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

feat: Resolve a npm package passed as --config #464

Merged
merged 1 commit into from Jun 11, 2018

Conversation

cameronhunter
Copy link
Contributor

@cameronhunter cameronhunter commented Jun 8, 2018

feat: Resolve a npm package passed as --config

This PR enables a npm package to be passed to the command line --config parameter and will be resolved by lint-staged.

Example:

Say you have a npm package my-tools-config/lint-staged, your application can now depend on the configuration package and lint-staged allowing:

lint-staged --config my-tools-config/lint-staged

@codecov
Copy link

codecov bot commented Jun 8, 2018

Codecov Report

Merging #464 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   98.72%   98.74%   +0.02%     
==========================================
  Files          12       12              
  Lines         235      239       +4     
  Branches       28       28              
==========================================
+ Hits          232      236       +4     
  Misses          3        3
Impacted Files Coverage Δ
src/index.js 92.5% <100%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d0ccb2...4a705f8. Read the comment docs.

@okonet
Copy link
Collaborator

okonet commented Jun 8, 2018

I’m wondering what are you trying to solve? 🚫💩 Lint-staged is meant to run on pre-commit so I’m not sure why it needs to be exposed to Node.

@cameronhunter
Copy link
Contributor Author

I understand your concern. We work on multiple applications using the same toolchain, it's a similar(-ish) setup to how Create React App encapsulates tools. Due to the reuse of our toolchain we keep our configuration files in a private npm module, tool-config.

Our pre-commit hook looks like:

#!/usr/bin/env node

const lintStaged = require('lint-staged');
const config = require('tool-config/lint-staged');

lintStaged(config);

This allows our configuration to be anywhere on the node resolution path, removing an absolute file path (that we had previously) and removes any breakage that would happen in a yarn monorepo (due to module hoisting).

@okonet
Copy link
Collaborator

okonet commented Jun 8, 2018

I’m just wondering is similar is possible with just using node’s resolve inside that file? I’m not against this change but before we go this route is like to check if it’s absolutely necessary. Also I’d like to avoid breaking changes if possible. Thoughts?

@cameronhunter
Copy link
Contributor Author

I guess we could use require.resolve on the --config param within lint-staged. It would allow us to change our pre-commit hook to:

#!/usr/bin/sh
$(npm bin)/lint-staged --config 'tools-config/lint-staged'

With regards to the break change: I was perhaps being overly cautious. I reordered the params on the internal interface which was never explicitly exposed (via package.json's main param). It would be a breaking change to people deep requiring into the module, e.g.:

const lintStaged = require('lint-staged/src/index');

But if they're doing that then this PR would be what they'd want to update to anyway.

@okonet
Copy link
Collaborator

okonet commented Jun 8, 2018

So I’m still not sure if this PR is needed :) can you confirm you can aichive this without it? Or it is still needed for your case?

@cameronhunter cameronhunter changed the title feat: Allow configuration to be passed to lintStaged JS API as an object feat: Resolve a npm package passed as --config Jun 8, 2018
@cameronhunter
Copy link
Contributor Author

Hi @okonet, I've updated the PR. I'm now using require.resolve on the --config parameter which enables a npm package to be used as config (as well as a file path as before).

This is no longer a breaking change and would only require a minor version bump.

sudo-suhas
sudo-suhas previously approved these changes Jun 9, 2018
okonet
okonet previously approved these changes Jun 9, 2018
@okonet
Copy link
Collaborator

okonet commented Jun 9, 2018

Can you please add a test for it?

@cameronhunter cameronhunter dismissed stale reviews from okonet and sudo-suhas via 4587aaf June 11, 2018 03:20
@cameronhunter
Copy link
Contributor Author

Test added

@@ -49,6 +49,13 @@ describe('lintStaged', () => {
expect(logger.printHistory()).toMatchSnapshot()
})

it('should load an npm config package when specified', async () => {
expect.assertions(1)
jest.mock('my-lint-staged-config')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't there be a mock implementation as well? Also, there should be an assertion to check that the specified package was resolved and required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is... the __mocks__/my-lint-staged-config directory is the mock implementation. The mock implementation contains the config { '*': 'mytask' } which is included in the snapshot providing an end-to-end assertion that the package was required and resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yes I see that now. My bad.

Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

LGTM

@okonet okonet merged commit c34a3f7 into lint-staged:master Jun 11, 2018
@okonet
Copy link
Collaborator

okonet commented Jun 11, 2018

Thanks for working on this!

@cameronhunter
Copy link
Contributor Author

Thanks for taking the time to review this, we ended up with a much cleaner solution than what I initially proposed. Can you speak to when this change will be published?

@sudo-suhas
Copy link
Collaborator

We use semantic-release so this has been released already via v7.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants