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

Logger updates: dependencies and higher minimum node version #1650

Merged
merged 2 commits into from Aug 25, 2023
Merged

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Aug 24, 2023

This is mainly intended as a discussion point.

I would like to do some updates in this repository. Recently the build for this repo was broken and this prompted me to look into it more. There are many issues with this repo, but I'd like to slowly chip away at improving things.

This PR is a template for what I would like to roll out to all the other non-deprecated packages in this repo. In short:

  1. Updating all the dependencies.
  2. Addressing any linter warnings.
  3. Bumping the minimum node version to v18 (v12 and 14 are already EOL, and 16 will be EOL in ~2 weeks).

I wanted to see what the team thinks of these changes? What are your opinions? Do you agree or disagree with any of these changes, in particular the raising of the minimum node version? And do you think this should warrant a major semver version increase?

Longer term, after these initial updates I would also like to tackle:

  • Updating the docs revisioned in this repository and possibly generating the reference documentation on every commit in CI, instead of manually.
  • Factoring out developer dependencies / tools, such as eslint, that are shared or common across all sub-packages, into the top-level package.json. Possibly this could leverage npm workspaces, though my initial foray into this area was unsuccessful, but I think mostly due to trying to support too many incompatible node versions at the same time.

@filmaj filmaj added discussion M-T: An issue where more input is needed to reach a decision pkg:logger applies to `@slack/logger` labels Aug 24, 2023
@filmaj filmaj requested a review from a team August 24, 2023 21:11
@filmaj filmaj self-assigned this Aug 24, 2023
@seratch
Copy link
Member

seratch commented Aug 24, 2023

When we drop older versions than 18, README (and some other document sections too?) needs to be updated accordingly: https://github.com/slackapi/node-slack-sdk#requirements

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

I don't see any issues with these changes. Left comment on the Node version requirement on other docs and README

@filmaj
Copy link
Contributor Author

filmaj commented Aug 25, 2023

I've added a README to this sub-package and made the node v18 requirement explicit. I will merge and release this and send similar pull requests for the other packages. In the next PR, I will update the root README.

@filmaj filmaj merged commit 783920c into main Aug 25, 2023
15 checks passed
@filmaj filmaj deleted the logger-major branch August 25, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision pkg:logger applies to `@slack/logger`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants