grab own package version from regular require #1442
Conversation
|
I wonder if the new |
Yeah exactly that happened when doing
|
|
Should be good to go now |
features/support/hooks.ts
Outdated
| @@ -28,7 +28,7 @@ Before(function ( | |||
| this.tmpDir = path.join( | |||
| projectPath, | |||
| 'tmp', | |||
| `${path.basename(pickle.uri)}_${line.toString()}` | |||
| `${path.basename(pickle.uri) ?? ''}_${(line ?? '').toString()}` | |||
There was a problem hiding this comment.
What's the need for this change?
There was a problem hiding this comment.
A new eslint rule that bubbled up
There was a problem hiding this comment.
I don't understand how that is possible. If you revert this does the build fail?
There was a problem hiding this comment.
We've recently upgraded the linter, which has new rules. Rules are only validated against modified files (that's how husky is set up).
So the linter only kicked in now that @davidjgoss modified the file.
Perhaps we should configure husky to lint/format everything? It will make commits a bit slower, but we avoid these kinds of surprised.
There was a problem hiding this comment.
What is husky? Can you point me to the commit that introduced that? I personally prefer to lint everything each build and think linting modified files only is only useful if you are starting linting and don't want to fix everything at once (though there are also other ways of dealing with that too, ie disable the rules with errors and enable each separately when fixing them all)
There was a problem hiding this comment.
AFAIK we lint all files not just changed - previously had to fix some new errors to help renovate do a eslint version bump for example. And the only change in this file is the one I made to fix the lint issue.
I can't quite figure out what has happened, but I wasn't getting the error locally, then saw it on Travis, then rm -r'd my node_modules and did a fresh yarn install, and then was able to see the error. This should be impossible because of lockfiles, unless some package is getting config from outside the versioned dependency tree.
There was a problem hiding this comment.
It's curious, but I don't think it's worthwhile spending more time understanding how this happened.
Is anything holding up merging this?
There was a problem hiding this comment.
Agree could be a rabbit hole. FWIW I do think it's a bit of an over-the-top rule.
There was a problem hiding this comment.
Can you please revert this change? I'd like to see the commit this error occur on CI? If it is still happening can we fix this in another PR? I'd like to narrow the discussion around it as I don't think both changes are needed
There was a problem hiding this comment.
Reverted - and of course now it looks to be passing.
Here's the build where it failed on this rule so I don't look totally mad.
|
Thanks @davidjgoss - are we ready to merge? |
|
@aslakhellesoy yep |
* grab own package version from regular require * use const not import * lint * lint * revert mysterious lint fix change
Fixes #1439