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

BREAKING CHANGE(core) run apps using node 14 #350

Merged
merged 3 commits into from
Mar 19, 2021
Merged

BREAKING CHANGE(core) run apps using node 14 #350

merged 3 commits into from
Mar 19, 2021

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Mar 18, 2021

I figured I'd do a dep update in a separate PR.

Also, I think we can take advantage of the travis build matrix feature to more cleanly declare our tests. On each of 12 and 14, run each of test and smoke test. I know you mentioned travis being slow; i'm not sure this will help a lot, but it'll at least clean up that file.

@xavdid xavdid requested a review from eliangcs as a code owner March 18, 2021 21:54
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

  • I left a question about Node 10 support.
  • GitHub Actions also provides Build Matrix, of which I think we can take advantage.

# the node-X segment in the next line should match the latest major version
zip -R $TARGET_FILE '*.js' '*.json' '*/linux-x64-node-10/*.node' '*/linux-x64-node-12/*.node'
# the node-X segment in the next line should match all supported node versions
zip -R $TARGET_FILE '*.js' '*.json' '*/linux-x64-node-12/*.node' '*/linux-x64-node-14/*.node'
Copy link
Member

Choose a reason for hiding this comment

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

Probably in a separate PR, we'll need to bump deasync in legacy-scripting-runner/package.json. The current version of deasync we're using doesn't have a Node 14 binary.

Also, as my other comment suggests, should we keep Node 10?

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, I'll grab that in my big update everything

@@ -159,7 +159,7 @@ const forceIncludeDumbPath = (appConfig, filePath) => {
return (
filePath.endsWith('package.json') ||
filePath.endsWith('definition.json') ||
filePath.endsWith(path.join('bin', 'linux-x64-node-10', 'deasync.node')) ||
filePath.endsWith(path.join('bin', 'linux-x64-node-12', 'deasync.node')) ||
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I understand AWS Lambda runtime support policy correctly. It seems Node.js 10 could still be running after it is deprecated on May 28. Does that mean we should keep both Node 10 and 12 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! so my understanding of how this works is that functions will keep running, but at some point we won't be able to update them anymore. This has mostly been fine for us, since a lot of our functions are long-lived.

But, that does mean if we update boilerplates on godzilla apps anytime after may, we'll need to also bump their node versions (and by extension, their platform versions). though, I suppose we could be building a new boilerplate between merging this and upgrading node. It's probably safest to include as many node binaries as we can, I can't imagine they're that big. I can tweak this and the other spot.

@xavdid
Copy link
Contributor Author

xavdid commented Mar 19, 2021

I've updated! This binary and version stuff is getting a little confusing, even for me 😅

@xavdid xavdid merged commit eb804d9 into master Mar 19, 2021
@xavdid xavdid deleted the PDE-2115 branch March 19, 2021 16:23
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

2 participants