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

fix failing test #44

Merged
merged 1 commit into from
Jul 3, 2021
Merged

fix failing test #44

merged 1 commit into from
Jul 3, 2021

Conversation

tjenkinson
Copy link
Contributor

Not sure how this passed on master? Dependency version change maybe?

Anyway this fixes it locally.

fixes #43

@eslint-github-bot
Copy link

Hi @tjenkinson!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

  • The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

@tjenkinson
Copy link
Contributor Author

There’s no package-lock so probably the yaml package changed. Maybe add a package lock?

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aladdin-add
Copy link
Member

@aladdin-add aladdin-add merged commit 593fbe3 into eslint:main Jul 3, 2021
@aladdin-add
Copy link
Member

weird! I didn't see a new js-yaml was released since last build. its deps?

@tjenkinson
Copy link
Contributor Author

tjenkinson commented Jul 3, 2021

🎉

fyi: https://github.com/eslint/eslint#why-doesnt-eslint-lock-dependency-versions

Ah ok. I get that the lockfile isn't used for dependencies for end users, but I still think it's valuable for reproducibility in dev and CI, and to prevent issues like this where a break is only noticed by chance. If a lockfile is used though then tools like dependabot/rennovate become important to highlight these breakages as new versions of dependencies are released. TypeScript also removed their lockfile at some point I think for similar reasons, but it's back again now.

weird! I didn't see a new js-yaml was released since last build. its deps?

🤷

@mdjermanovic
Copy link
Member

I think this is the cause: kettanaito/fs-teardown@dfc1600

Published 21 days ago https://www.npmjs.com/package/fs-teardown

@tjenkinson
Copy link
Contributor Author

Ah nice find that looks like it would remove the indentation from the first line only. I wonder why they added that 🤔

@kettanaito
Copy link

The fs-teardown package has introduced trimming of file's contents but didn't account for that being a breaking change. Thanks to the cooperation from @tjenkinson, the change has been reverted and published in fs-teardown@0.1.3.

@mdjermanovic
Copy link
Member

Thanks for the quick update!

It seems that fs-teardown@0.1.3 introduced another breaking change by setting "fs-extra": "^10.0.0" (kettanaito/fs-teardown@3e75dd6), which dropped support for Node 10:

https://github.com/jprichardson/node-fs-extra/blob/e6a95058c930953113177c9518f57e83cace3e79/package.json#L6

The latest Node 10.x seems to work, but our CI is failing on Node 10.12.0:

     TypeError: Cannot read property 'writeFile' of undefined
      at /home/runner/work/eslintrc/eslintrc/node_modules/fs-teardown/lib/index.js:2854:73
      at step (node_modules/fs-teardown/lib/index.js:90:23)
      at Object.next (node_modules/fs-teardown/lib/index.js:71:53)
      at fulfilled (node_modules/fs-teardown/lib/index.js:61:58)

(off-topic, since the error is on line 2854, it might be that fs-teardown inadvertently bundles dependencies?)

We are in the process of preparing a major release that will drop Node 10, so we could pin fs-teardown to a previous version for now to fix our CI, and un-pin once we start with prereleases.

@kettanaito
Copy link

Hey, @mdjermanovic. Thank you for pointing out that breaking change in fs-teardown. I'm preparing the next release of the library and wanted to align with you whether you've already dropped Node.js 10 support in ESLint?

If not, I can try to ensure that the new release of fs-teardown is compatible with your current supported Node.js version. I believe ESLint can benefit from the reworked API I'm about to publish. Please let me know.

@mdjermanovic
Copy link
Member

Hi @kettanaito!

Yes, we dropped Node.js 10. The versions we support now are:

eslintrc/package.json

Lines 77 to 79 in be0f70d

"engines": {
"node": "^12.22.0 || ^14.17.0 || >=16.0.0"
}

We have switched to "fs-teardown": "^0.1.3" and everything seems to be working fine.

@kettanaito
Copy link

@mdjermanovic, fantastic!

There's a reworked API in the next version of fs-teardown, and I'd like to integrate it into this project's testing setup. Would you have some time to help me with the code review once I open the pull request? Thanks.

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 21, 2021

There's a reworked API in the next version of fs-teardown, and I'd like to integrate it into this project's testing setup. Would you have some time to help me with the code review once I open the pull request? Thanks.

@kettanaito sorry, I missed this. Would you like to open an issue with the description of the new API, so we could discuss the changes?

@kettanaito
Copy link

Hey, no worries. The next API has been merged here and you can browse through it in the README. I'd love to open a pull request that updates fs-teardown in ESLint once I have time.

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

Successfully merging this pull request may close these issues.

"ConfigArrayFactory: loading config files should work properly" test failing
4 participants