slackapi / node-slack-sdk

Slack Developer Kit for Node.js
https://slack.dev/node-slack-sdk
MIT License
3.27k stars 660 forks source link

New project structure: Separate WebClient, RTMClient packages; monorepo #677

Closed aoberoi closed 5 years ago

aoberoi commented 5 years ago

Description

The project maintainers would like to propose a new structure that would affect this package, the @slack/interactive-message package, and the @slack/events-api package.

Goals

  1. Make fewer destinations for people who use these packages to reach the rest of the community and the maintainers. We've noticed that these packages are often used together (as we intended). But if you run into an issue and need some help, it can be needlessly confusing and sometimes impossible for you to understand where the best place is to post an issue.

  2. Reduce build, test, and documentation tooling cost. Each of the packages have their own build system, documentation generation, testing tooling baked into the repo. A large portion of this is duplicate effort to maintain. This cost seems to be growing as the community has made it clear we need type definitions for both @slack/events-api and @slack/interactive-messages (e.g. https://github.com/slackapi/node-slack-events-api/pull/52). The documentation and testing tooling has varying levels of completeness, and we should make it easier for all packages to improve.

  3. Minimize changes for existing users. To the extent that its possible, any changes should allow existing users to continue using these packages without needing to change their code. We want existing users to continue to take advantage of updates and advancements.

  4. Foundation for shared abstractions and code reuse. There are various data structures and code that are duplicated in several places, and we'd like to create a structure that makes it possible to consolidate those. For example, the request signing code for the @slack/events-api and @slack/interactive-messages packages can be shared (or abstracted to a common dependency that can independently be used).

Proposal

There's two parts of this proposal:

  1. Move code from the @slack/interactive-message and @slack/events-api GitHub repos into this repo. This does not mean those packages would be published in the @slack/client package (see below), but rather that we'd have a monorepo that hosts code for multiple packages. This allows the documentation, test, and build tooling to be consolidated, but will require additional tooling to support publishing multiple packages from the same code repository. Maintainers will need to adjust the publishing workflow.

  2. Decompose the @slack/client package into three independent packages: @slack/web-api, @slack/rtm-api, and @slack/webhook. Continue to publish @slack/client as an umbrella package that simply depends on those three, and exports the same values as it does today. Adjust the documentation to point to the specific package names, so that new users can load only the code their application needs (reduce bundle size, cold-start time). It also allows the community to inspect the relative popularity of the individual parts of the Slack Platform based on npm download counts.

The end state after this change is we publish 6 npm packages from this repo: @slack/web-api, @slack/rtm-api, @slack/webhook, @slack/events-api, @slack/interactive-messages, and the umbrella package @slack/client. The other GitHub repos would become archived, and we'd open new issues to mirror all the open ones (or consolidate them into existing ones if they are the same). Our triage/label system would evolve to include package:X as metadata to determine which packages an issue or PR affects.

These changes are large enough that I think we should release them along with the next major version of @slack/client, even though we don't see this to be the cause for any breaking API changes.

Next Steps

What do you think?

We're looking for feedback on this proposal, and would appreciate your comments. Is this change exciting and beneficial to you? Can you anticipate any problems we haven't already listed?

Happy 2019 everyone 🎉

Requirements (place an x in each of the [ ])

clavin commented 5 years ago

I love this idea! I've thought it over a couple times in the past (see #330 and #410). In my time thinking, here's some possibly useful conclusions I've drawn:

  1. Many strategies and notes for maintaining a monorepo should be taken/inspired from popular projects that are monorepos, such as babel and React
  2. Babel has a good read on why it's structured as a monorepo. This SDK's case of juggling so many packages harmonizes perfectly with others' reasons for being monorepos.

Working with lerna

First, a practical description, because it took me (personally) a longtime to grasp what it actually does. Lerna essentially boils down to a cli app that helps developers do a couple of tasks across a bunch of packages in a single monorepo:

What lerna does not do is provide or facilitate similar package structures. This means lerna doesn't help reduce the TypeScript boilerplate required across the packages in this monorepo. The typical response to this is to have a root-level tsconfig.json, and then one in each package that extends from the root tsconfig.json.

But what if you add another tool, and another tool? You can easily find yourself having configuration files for TypeScript, TSLint, Prettier, Testing Coverage, and more, in each package. This might be feasible for the mere six or seven packages outlined in this issue, but it becomes a problem when we want to add new packages. I can definitely see myself asking "did I forget any config files?" if I were to ever add a package to a monorepo like this. This is also somewhat in contrast with goal 2.

@slack/server and "group" packages

I'm going to hold the idea of merging the events API & interactive messages packages. I'm not particularly fond of @slack/server for a few reasons:

  1. Looking at the 6 proposed packages in this issue, all 6 seem equally valid to be in server code for Slack apps, meaning @slack/server would end up just being a kind of "group" of all SDK packages.
  2. The name might be confusing to new developers, who might mistake the name as being an implementation of Slack's app server

I have a somewhat controversial suggestion that's completely up for debate:

Rename @slack/client to @slack/sdk

Here's my elevator pitch:

There's one obvious con: migrating developers from @slack/client to @slack/sdk. Making a change like this could easily become the cherry on the cake of migration tasks that keeps developers from upgrading--even if it's completely facilitated by a simple codemod. This also somewhat aligns with the fact that this would be a major semver change.

I feel it might be more likely that @slack/client continues to be supported, probably just reexporting from @slack/rtm-client and @slack/web-api (etc.), whereas @slack/sdk can be better fit as the umbrella package, exporting from and linking together the major packages (whichever those may be).

Events and Interactions

This could, maybe ideally, be separated into another issue that's dealt with after the monorepo is initially set up

I feel like there's a good point raised, that the Events API and Interactive Messages API are nearly the same model:

  1. Something happens: a user interacts clicks a button, leaves a channel, pins a message, etc.
  2. Slack sends a payload to your server
  3. Your server responds to the payload request with a 200 OK (interactive messages have a slightly different response protocol)
  4. Handle the incoming event
  5. ...which might include firing off a Web API method or two

There are some key differences with how developers can use these APIs:

Difference Interactive Messages API Events API
HTTP Response 200 OK, but allows: in-conversation messages, ephemeral messages, no message 200 OK with a challenge sometimes. No response message, not even ephemeral messages.
Action Response response_url or Web API Only Web API

I feel like the name for the merging of these two packages mostly depends on what the API that respects the differences of the APIs while still managing their common behavior looks like in practice. I was thinking @slack/incoming, but that's likely to be confused with incoming webhooks. Maybe just @slack/middleware?

Going forward

Some other topics that came to my mind:


All in all, I feel like this is definitely the right direction for this project! I was previously hoping to propose something like this during my summer internship, but now I'm hoping to get to work on it 😄.

aoberoi commented 5 years ago

thanks for your thoughts @clavin! i'm glad to see that we've got a similar vision. i wanted to respond to some of your comments directly, so here we go.

Thanks for doing the research and sharing what you learned about... lerna.

The typical response to this is to have a root-level tsconfig.json, and then one in each package that extends from the root tsconfig.json. [...] But what if you add another tool, and another tool? You can easily find yourself having configuration files for TypeScript, TSLint, Prettier, Testing Coverage, and more, in each package.

I think this is true, but in many cases a single configuration is all that we will need. For example, if we standardize on TSLint for linting across each of the packages, then a single tslint.json can specify a set of files that include all packages (through a combination of globs, excludes, tsconfig, etc). In fact, I think this will extend to almost all of these tools. Even if we have child configs for individual packages, the inheritance seems like a win because we'd only have to define the differences from one config to another, and most settings will be common.

I'm being optimistic, but I think all devDependencies could likely be moved up to a parent package.json, and then the individual packages might not need any.

I'm not particularly fond of @slack/server for a few reasons [...] I feel like the name for the merging of these two packages mostly depends on what the API that respects the differences of the APIs while still managing their common behavior looks like in practice. I was thinking @slack/incoming, but that's likely to be confused with incoming webhooks. Maybe just @slack/middleware?

Points taken. My thought process behind that name is that while the WebClient, RTMClient, and IncomingWebhook objects all behave like HTTP clients (they make requests), the SlackEventAdapter and SlackMessageAdapter behave like HTTP servers (and also support being used as a listener/middleware in an existing HTTP server). In my mind, the simplest distinction is "who makes the request" vs. "who serves the request." I think @slack/incoming sounds a bit unintuitive (inventing new terminology?). I think @slack/middleware sounds better, but middleware is a pretty specific concept mostly associated with express/connect in Node. The package actually offers a requestListener in terms of the standard library, which is a more generic construct (all requestListeners are middleware but not all middleware are requestListeners).

For now, I'll leave this point open for discussion. We don't have a strong vision for the future roadmap for those packages, so I'd prefer to make as little change as possible until we know its benefits. I see benefit in factoring out the verifyRequestSignature for reuse, but that's about all right now.

Rename @slack/client to @slack/sdk [...] Making a change like this could easily become the cherry on the cake of migration tasks that keeps developers from upgrading. [...] @slack/sdk can be better fit as the umbrella package, exporting from and linking together the major packages (whichever those may be).

I understand this rationale, and if I thought there was a need for distributing the collection of classes as one package, I would agree! But actually, I'd rather push developers to install only the bits they need to use. We should make that the happy path, and make that easier (lower bundle size, cold start time). This also has the benefit of getting more granular stats from npm (if you haven't observed the rise in downloads, its pretty amazing) - which helps maintainers understand the impact of changes we make and overall trends for our users without putting any "phone home" code in the runtime.

PS. I do however think there are opportunities in grouping functionality if it wasn't simply a collection of classes, but instead a higher-level framework with opinions and a more complete solution out of the box. But these packages are destined to fill a different purpose: relatively low-level utilities, with broad application (by keeping controversial opinions to a minimum).

PPS. There's plenty of interesting ideas like ☝️ and more, we won't run out before the summer 😉 .

aoberoi commented 5 years ago

Looks like the TypeScript project will be contributing to ESLint, and plan for it to supercede TSLint. (see "Linting" header https://github.com/Microsoft/TypeScript/issues/29288)

clavin commented 5 years ago

My thought process behind that name is that while the WebClient, RTMClient, and IncomingWebhook objects all behave like HTTP clients (they make requests), the SlackEventAdapter and SlackMessageAdapter behave like HTTP servers (and also support being used as a listener/middleware in an existing HTTP server). [...] I think @slack/incoming sounds a bit unintuitive (inventing new terminology?).

Your distinction between client & server sound a lot like the Features section of the current readme, which (now that I look at it) also already uses the terminology "incoming" and "outgoing". 🤷‍♀️ But I digress, it doesn't quite feel right as a package name.

Otherwise, good points, notes taken! 👍

One last thing I totally forgot about: versioning! Would the monorepo use fixed versioning or independent versioning? I personally like the idea of fixed versioning, and would be happy to see it here; however, I haven't quite yet thought out the pros and cons of both.

aoberoi commented 5 years ago

Great new npm feature that we should take advantage of: https://github.com/npm/rfcs/blob/latest/implemented/0010-monorepo-subdirectory-declaration.md

aoberoi commented 5 years ago

https://github.com/typicode/husky/blob/master/DOCS.md#multi-package-repository-monorepo