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

Make slack-sdk compatible with node 10 #582

Closed
5 of 9 tasks
kbiedrzycki opened this issue Jun 13, 2018 · 8 comments · Fixed by #605
Closed
5 of 9 tasks

Make slack-sdk compatible with node 10 #582

kbiedrzycki opened this issue Jun 13, 2018 · 8 comments · Fixed by #605
Labels
area:typescript issues that specifically impact using the package from typescript projects

Comments

@kbiedrzycki
Copy link

kbiedrzycki commented Jun 13, 2018

Description

We're trying to use slack-sdk with projects which uses newer version of @types/node package (> 10.0.x). Slack is depending on ^9.4.7 which causes dependency collision and prevents us from successfully building package. Is it possible to make slack-sdk @types/node version agnostic, for example using @types/node: '*'?

What type of issue is this?

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi
Copy link
Contributor

aoberoi commented Jun 13, 2018

Thanks for raising this point! I'm not certain what the right solution would be at this point.

If we were to bump the version to 10.x, then another consume using a lower version of node would likely end up in the same position that you are in now.

If we were to move @types/node to the devDependencies, then any values returned by this package's APIs that are derived from node's types would not be defined for consumers.

If we were to remove @types/node all together, the problem above would still occur.

It seems like the best option might be to use * for the version specifier as you suggested, but I haven't found any other examples of this so I am hesitant. I'm going to ask around to get more opinions.

@kbiedrzycki
Copy link
Author

So currently we finished up with making fork and changing @types/node version to * which is resolved properly now. There's also another way - we can manually update package.json file with resolutions section and force other version of node, I guess that will do the trick as well. Anyway, if you find out some other solution, more universal, we will be very happy about that.

@aoberoi
Copy link
Contributor

aoberoi commented Jun 14, 2018

@kbiedrzycki thanks for the data point! I also want to leave a reference to #514, an issue where this was discussed in the past.

@aoberoi aoberoi added the area:typescript issues that specifically impact using the package from typescript projects label Jul 3, 2018
aoberoi added a commit to aoberoi/node-slack-sdk that referenced this issue Aug 7, 2018
@clavin
Copy link
Contributor

clavin commented Aug 8, 2018

One solution might be to just set @types/node as a peerDependency set to >=8.0.33 or so.

  • Those who don't use TypeScript don't actually have to install the package (though they do have an annoying warning saying its not installed)
  • Those who do use TypeScript are ensured that a compatible version is used

@aoberoi
Copy link
Contributor

aoberoi commented Aug 8, 2018

@clavin IMHO peerDependencies don’t really provide much value beyond emitting a warning. For many users (non TS) the warning adds to confusion. For the others (TS) it’s not enough; easy to ignore and doesn’t actually force npm to install anything. That solution is roughly equivalent to removing @types/node from package.json

@clavin
Copy link
Contributor

clavin commented Aug 8, 2018

Yeah 😕... this might just be a tooling issue that has no solution, as it currently stands. I feel like there'd need to be some kind of typeDependencies magic going on with TypeScript/npm for this issue to be resolved.

According to this SO answer, the way the dependencies are as of right now are correct.

Maybe the current situation can be aided by changing the supported @types/node version to be a minimum compatibility (>=x.x.x) range, rather than an equal compatibility (^x.x.x) range?

@aoberoi
Copy link
Contributor

aoberoi commented Aug 8, 2018

@clavin typeDependencies is an interesting idea. I’d like to get that in front of some of the TypeScript core team to see what they think.

And yep, the minimum compatibility range is what I’ve implemented in #605. I think this range should be okay because we no longer depend on typeof util.callbackify actually, we still do. However, this seems like a good reason not to.

@aoberoi
Copy link
Contributor

aoberoi commented Aug 8, 2018

@clavin i made some changes to my PR. i've copied the type of util.callbackify into this project so that we no longer depend on the definition coming from @types/node, to mirror the fact that we don't depend on the implementation being in node. this isn't the perfect solution, but i think it will be better for the project and its users than the alternatives we've explored. i'd appreciate your feedback, if you have any concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants