-
-
Notifications
You must be signed in to change notification settings - Fork 412
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
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, Our #!/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). |
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? |
I guess we could use #!/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 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. |
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? |
--config
Hi @okonet, I've updated the PR. I'm now using This is no longer a breaking change and would only require a minor version bump. |
Can you please add a test for it? |
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') |
There was a problem hiding this comment.
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 require
d.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for working on this! |
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? |
We use |
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: