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

migrate to typescript & node.js-compatible es modules #241

Closed
wants to merge 5 commits into from
Closed

migrate to typescript & node.js-compatible es modules #241

wants to merge 5 commits into from

Conversation

zackschuster
Copy link
Collaborator

@zackschuster zackschuster commented Sep 18, 2019

  • port to node.js-compatible es modules
  • port to typescript
  • reconfigure rollup
  • fix tests
  • point main to either cjs rollup bundle or esm source (prob cjs)

@eleith
Copy link
Owner

eleith commented Sep 18, 2019

looking good.

i'm also open to seeing this project move to typescript if that helps with big changes like this to prevent regressions.

@zackschuster
Copy link
Collaborator Author

i wouldn't mind doing the port job to typescript. i'd also move the test suite to ava, which i have experience in using with typescript.

@zackschuster
Copy link
Collaborator Author

PR is in typescript now. made a few (relatively) minor API changes (like condensing error codes into a const enum & getting rid of legacy code that said "will remove at some point"), since i'm pretty sure this will end up semver major no matter what.

@zackschuster zackschuster changed the title Adopt ECMAScript Modules migrate to typescript & target --experimental-modules Sep 18, 2019
@eleith
Copy link
Owner

eleith commented Sep 18, 2019

i wouldn't mind doing the port job to typescript. i'd also move the test suite to ava, which i have experience in using with typescript.

why ava vs jest?

@zackschuster
Copy link
Collaborator Author

technically, they have a pretty similar feature set & both are written with current tools. the breaker for me is that jest is a facebook project, whereas ava is community-led (started by sindresorhus).

@zackschuster
Copy link
Collaborator Author

also it looks like we'd have to install @types/jest, whereas ava has types built-in: https://swizec.com/blog/the-big-mac-index-and-jest-fetch-testing/swizec/9187

(coincidentally i've already tested fetch with ava)

@zackschuster zackschuster changed the title migrate to typescript & target --experimental-modules migrate to typescript & node.js-compatible es modules Sep 19, 2019
@zackschuster
Copy link
Collaborator Author

esm in node will probably ship unflagged on 12.x in april. i'll try to finish this around then.

@zackschuster
Copy link
Collaborator Author

tests are failing locally due to smtp server issues. image attached for reference:
image

@zackschuster
Copy link
Collaborator Author

zackschuster commented Apr 21, 2020

i think the above might be due to changes i made during conversion. i'm going through the shipped code to repair differences.

side note: node 14 today! hooray for unflagged modules! fun fact: the flag warning almost wasn't removed & node's tsc was surprised to see it happen

@zackschuster
Copy link
Collaborator Author

modules are still considered experimental, so i feel less bad about not landing this in time with the launch >D

@eleith
Copy link
Owner

eleith commented Apr 22, 2020

@zackschuster looking good. i've given you direct access to the project in reflection of your contribution to the project. fyi, a few of the outdated dependencies for emailjs are throwing up security alerts on github for this project, so perhaps those can get addressed during this upgrade.

@zackschuster
Copy link
Collaborator Author

thank you! :D i'll see if i can move the PR from my fork to a local branch without breaking things.

for addressparser, would you like to switch to emailjs-addressparser?

@eleith
Copy link
Owner

eleith commented Apr 22, 2020

depends on the effort level, if it is transparent, sure.

@zackschuster
Copy link
Collaborator Author

closing in favor of #246

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.

2 participants