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

Document email adapter #1144

Merged
merged 8 commits into from
Mar 25, 2016

Conversation

drew-gross
Copy link
Contributor

Lets document this and get it out of experimental. We really shouldn't let features be experimental for too long.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

appName: 'Parse App',
// The email adapter
emailAdapter: {
module: 'parse-server/lib/Adapters/Email/SimpleMailgunAdapter',
Copy link
Contributor

Choose a reason for hiding this comment

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

that is so really ugly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Thoughts on splitting it out into it's own npm package? Then we would have no email adapters in parse-server package which has upsides and downsides.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have it as a dependency. With our adapter loader, we need The full module. Can't handle variables which can be problematic too.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be consistent for all adapters actually. And with npm a parse-server dep don't get exposed to the main package...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep have it as a dep then just have

emailAdapter: {
  module: "parse-server-simple-mailgun-adapter",
  options: {...}
}

And it will just work, and if you want to use a different email adapter you just have to add it to your package.json. We can do this for all adapters. For variables you can just require it explicitly and provide an instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let's do 1 repo with all the adapter source and n package.json. What do you think?

Maybe keep the source in parse-server but in a separate folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would 1 repo with all the adapters work for requiring? Then you can't use the initialization syntax I described above.

Keeping the source in parse-server seems messy if it's in a different npm package. One to one mapping between repos and packages seems like the only sane solution to me.

@codecov-io
Copy link

Current coverage is 92.63%

Merging #1144 into master will increase coverage by +0.11% as of 7404639

@@            master   #1144   diff @@
======================================
  Files           90      89     -1
  Stmts         5633    5621    -12
  Branches      1015    1014     -1
  Methods          0       0       
======================================
- Hit           5212    5207     -5
  Partial         11      11       
+ Missed         410     403     -7

Review entire Coverage Diff as of 7404639

Powered by Codecov. Updated on successful CI builds.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

2 similar comments
@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@drew-gross drew-gross force-pushed the document-email-adapter branch from 2c397a4 to ecf65ba Compare March 24, 2016 01:53
@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@flovilmart
Copy link
Contributor

LOVE IT!

@drew-gross drew-gross force-pushed the document-email-adapter branch from ecf65ba to b4ee313 Compare March 24, 2016 21:42
@drew-gross
Copy link
Contributor Author

Rebased

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@flovilmart
Copy link
Contributor

Don't you wanna call it parse-server-MailGun-adapter?

@drew-gross
Copy link
Contributor Author

I put simple in so that Mailgun could make an official one if they want.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@drew-gross drew-gross merged commit 7738d33 into parse-community:master Mar 25, 2016
@drew-gross drew-gross deleted the document-email-adapter branch April 5, 2016 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants