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

Repo now out of date #478

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Repo now out of date #478

wants to merge 13 commits into from

Conversation

dooley1001
Copy link
Contributor

@dooley1001 dooley1001 commented Jan 2, 2024

Description

With the release of Node v18, Node is migrating over from the old CommonJS (CJS) standard to the newer, ES Modules (ESM) standard. As a result the current snippets are out of date - https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

More info/context - https://www.twilio.com/docs/serverless/functions-assets/faq#how-can-i-use-an-es-module-in-my-function

Checklist

  • I ran npm test locally and it passed without errors.
  • I acknowledge that all my contributions will be made under the project's license.

This script will forward a incoming MMS image as an attachment to a email using Sendgrid.
Added warning in README
@dooley1001 dooley1001 changed the title Repo now redundant Repo now out of date Jan 10, 2024
Updated README with waring message.
Touched up appearance of the URL
Copy link
Collaborator

@BlakePetersen BlakePetersen left a comment

Choose a reason for hiding this comment

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

Couple small items 🙇

forward-message-sendgrid/functions/forward-mms-sendgrid.js Outdated Show resolved Hide resolved
forward-message-sendgrid/functions/forward-mms-sendgrid.js Outdated Show resolved Hide resolved
forward-message-sendgrid/README.md Outdated Show resolved Hide resolved
Edited "SMS" to "MMS"
Changed "Blog" to  "blog"
@makserik
Copy link
Contributor

Could you please add the out of date info to the main readme as well? Thank you

@dooley1001
Copy link
Contributor Author

dooley1001 commented Jan 15, 2024

I believe I have already @makserik - 4f87ee5

"ecosystem are migrating" to "ecosystem is migrating"
@makserik
Copy link
Contributor

I believe I have already @makserik - 4f87ee5

I see it only under forward-message-sendgrid/README.md

@dooley1001
Copy link
Contributor Author

I see what you mean now. My bad.

Does this work @makserik ? - #501

@makserik
Copy link
Contributor

makserik commented Jan 25, 2024

@dooley1001 thank you, I have replied to #501 . I believe the warning here might also be redundant. Otherwise it looks good. (Note I have not checked if the function works, we trust the owning team to do that)

I would also encourage you to check out the GH actions build results

Warning redundant and removed. Simply just included text stating repo out of date
@dooley1001
Copy link
Contributor Author

No worries @makserik - I have removed the "warning" but left the main text. Are we all good to merge ?

Removed the words "out of date" and clarified the JS migration
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.

None yet

3 participants