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

Updated sendmail.js to ES2015 | Updated Readme.md to ES2015 | Updated… #27

Merged
merged 1 commit into from
Dec 17, 2016

Conversation

CherryNerd
Copy link
Contributor

@CherryNerd CherryNerd commented Dec 5, 2016

Updated to ES2015

Description

All var statements are updated to const and let.
Semicolons(;) have been added to all lines, since it's easier for the Javascript parser to work with.
I have not updated callback functions to arrow functions, since it could change the this context and it could possibly break the project. This should be done after tests are written, to make sure it's all still working.

This is a "breaking" change, since a pre ES2015 env would crash on the const and let usage.

How Has This Been Tested?

Uploaded code to own server and send mails to myself.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes. (No tests have been written so far)
  • All new and existing tests passed. (No tests have been written so far)

@CherryNerd CherryNerd mentioned this pull request Dec 5, 2016
@CherryNerd
Copy link
Contributor Author

Related to #25

@GreenPioneer
Copy link
Collaborator

I am gonna bump this to version 2.0.0 because for the people still on older versions of node this will break it for them. This is mainly for the enterprise users out there.

@CherryNerd
Copy link
Contributor Author

No problem, just remember that more code changes could lead to this commit / PR being worthless, since it'll need to be done all-over again ;)

@GreenPioneer
Copy link
Collaborator

GreenPioneer commented Dec 6, 2016

Are you saying that because people will want 1.1.0 upgraded too?

@CherryNerd
Copy link
Contributor Author

What I meant is that if another thing like the DKIM implementation is added, this PR will no longer be conflict free and it's possible what I did needs to be re-done.

For example:

Original:

1| var foo = 100;
2| foo = foo.toString();
3| var bar = foo + 'baz';

PR:

1| let foo = 100;
2| foo = foo.toString();
3| const bar = foo + 'baz';

Added module in between

1| var foo = 100;
2| foo = process.env.NODE_ENV === 'production' ? foo.toString() : 'qux';
3| var bar = foo === 'quz' ? 'dev mode' : foo + 'baz';

It's a bit random what I did in the last part, but I wanted to make clear that the PR doesn't add up with the original anymore if there is a new module (or something else) added in between, e.g. for testing purposes.

@CherryNerd
Copy link
Contributor Author

Also, small comment: when a package is published as a certain version number, it's impossible with NPM to publish something else with the same version number.
For example, if you want the changes I made for #26 to appear on npm, it'll have to be under a different version number than 1.1.0, for example under v1.1.1

@GreenPioneer
Copy link
Collaborator

Yea get what your saying - following Semantic Versioning though deems that we need to bump it pass 1.x to 2.x because we will stop support Node 0.6.x - 0.12.x which we currently do and most people dont know it.

To the point of your PR ill get it in tonight or in the morning so that its not worthless

@CherryNerd
Copy link
Contributor Author

CherryNerd commented Dec 6, 2016

You're 100% right on the Semantic Versioning part, that's also why I marked it as a breaking change in the initial PR comment and the comment I made about #26.
And yes, I know 1.x support 0.6x - 0.12.x as I have also updated the package.json with the new node engine requirement.

Thank you for checking out my PR!

@GreenPioneer
Copy link
Collaborator

GreenPioneer commented Dec 6, 2016

I was working with the code a little tonight cause if were going es6 we might as well analyze the code a little more and see what we can make more efficient in es6. It did make me think .... that it would be smarter if i take one step back and write some baseline test cases while we know everything is working great #28 . Im gonna get started on it tomorrow unless you have something already in mind @Geex-Renzo

@CherryNerd
Copy link
Contributor Author

Going to respond to #28 in a bit, gimme a sec

@CherryNerd
Copy link
Contributor Author

@GreenPioneer Have you seen my comment at #28 ?

@GreenPioneer
Copy link
Collaborator

Yea @Geex-Renzo i saw. With it being the holiday ill try to get to it asap

@GreenPioneer GreenPioneer merged commit 2e120b1 into guileen:master Dec 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants