Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

feat(unsubscribe): merge n1-unsubscribe plugin #49

Merged
merged 2 commits into from
Jul 30, 2017

Conversation

KyleKing
Copy link

This PR will merge the n1-unsubscribe plugin into an internal plugin within nylas-mail

The plugin both unsubscribes and archives emails with a single button press. Users have the option of setting the default action to trash emails instead of archiving, to set a keyboard shortcut, add an optional confirmation dialog, and there is decent multilingual support.

The PR to merge the unsubscribe plugin is well overdue and will close nylas/nylas-mail#3154, which happened right at the cusp of the N1 > Nylas Mail transition and the PR that occurred right at the official Nylas Team > OS Community transition: nylas/nylas-mail#3612

KyleKing added 2 commits July 14, 2017 23:55
Integrate n1-unsubscribe into nylas-mail directly
See: https://github.com/colinking/n1-unsubscribe
and fix out-of-date “N1” references with “Nylas Mail”
Copy link
Member

@dweremeichik dweremeichik left a comment

Choose a reason for hiding this comment

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

I'm 👍 on this, just curious about the listener change.

@@ -153,7 +153,7 @@ module.exports =
setupEmitter: ->
return if @_emitter
@_emitter ?= new EventEmitter()
@_emitter.setMaxListeners(50)
@_emitter.setMaxListeners(100)
Copy link
Member

Choose a reason for hiding this comment

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

Just curios why the listener limit needed to be raised?

Copy link
Author

Choose a reason for hiding this comment

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

@dweremeichik I had to raise the maximum to allow more for the unsubscribe plugin. There doesn't appear to be any memory leaks and is just the nature of how the plugin was implemented

@mikeseese
Copy link
Contributor

@KyleKing could you make an issue on this repo for this? I know you already have two (that you have most graciously linked), but due to preservation sake I'd prefer the redundancy. Thanks!

On another note, as a built in plugin, this should not change any behavior for deleting or archiving emails correct? This should be supplemental buttons/keymaps that allow this enhanced feature.

@dweremeichik
Copy link
Member

@seesemichaelj the code appears to be scoped to the "new" internal unsubscribe package.

Copy link
Contributor

@mikeseese mikeseese left a comment

Choose a reason for hiding this comment

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

after the question from @dweremeichik is answered and he's good with this, this has my approval

@KyleKing
Copy link
Author

Wow, Nylas Mail is still alive! Thanks for the quick responses. The plugin will call the Nylas API for archiving or deleting, so there is no impact on any of the default Nylas behavior

@colinking and I are both welcome to input, so let us know what can be improved

Copy link
Member

@dweremeichik dweremeichik left a comment

Choose a reason for hiding this comment

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

@KyleKing the community is definitely trying to keep Nylas mail alive. Awesome work and initiative on bringing this plugin into the main project! I'm good with merging this in. Please feel free to join us in slack or other issues in the repo!

@dweremeichik dweremeichik merged commit f57b138 into nylas-mail-lives:master Jul 30, 2017
mikeseese pushed a commit that referenced this pull request Sep 15, 2017
Moves the unsubscribe plugin into an internal package.
* init: `unsubscribe` as internal package

Integrate n1-unsubscribe into nylas-mail directly
See: https://github.com/colinking/n1-unsubscribe

* fix broken N1 api request

and fix out-of-date “N1” references with “Nylas Mail”
mikeseese pushed a commit that referenced this pull request Sep 15, 2017
Moves the unsubscribe plugin into an internal package.
* init: `unsubscribe` as internal package

Integrate n1-unsubscribe into nylas-mail directly
See: https://github.com/colinking/n1-unsubscribe

* fix broken N1 api request

and fix out-of-date “N1” references with “Nylas Mail”
simonft pushed a commit to simonft/nylas-mail that referenced this pull request Sep 29, 2017
…l-lives#49)

Moves the unsubscribe plugin into an internal package.
* init: `unsubscribe` as internal package

Integrate n1-unsubscribe into nylas-mail directly
See: https://github.com/colinking/n1-unsubscribe

* fix broken N1 api request

and fix out-of-date “N1” references with “Nylas Mail”
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants