Skip to content

Bump dependencies / ESM / bundle with esbuild#36

Merged
elprans merged 3 commits intogeldata:mainfrom
CarsonF:main
Oct 13, 2023
Merged

Bump dependencies / ESM / bundle with esbuild#36
elprans merged 3 commits intogeldata:mainfrom
CarsonF:main

Conversation

@CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Oct 9, 2023

  • Run on Node v12 -> v20
  • Bundle with esbuild instead of @vercel/ncc
    Never even heard of ncc until this, not really sure their purpose.
    esbuild is definitely one of the new standards.
  • Bump all libraries (solves legacy @actions/core warnings set-output is deprecated #34)
  • Opted into more ESM.
    Though the build is still CJS, as that's what @actions/core emits for now.
    Some work went into mocking ESM modules for Jest, but I believe the result is a nicer experience anyways.
  • Bump some of the edgedb package versions referenced in tests to more recent ones
  • Support (forward) arm64 (aarch64) architecture

Fixes #34

I really tried to limit any logical changes here, since this is so large in other ways.
I would like to refactor further in later PRs if allowed.

@elprans
Copy link
Member

elprans commented Oct 10, 2023

Tests seem to be failing with

ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and '/home/runner/work/setup-edgedb/setup-edgedb/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 10, 2023

Thanks for approving CI. I had a hard time with the test suite since there's not a binary for M1/ARM chips.
Let me address CI failures.

@elprans
Copy link
Member

elprans commented Oct 10, 2023

there's not a binary for M1/ARM chips

Which binary? Both server and CLI have native m1 and linux aarch64 builds (though possibly not ancient releases that might be referred to in the tests, so try using a more recent server/CLI version there).

@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 10, 2023

● setup-edgedb › Installs CLI

This action does not support the arm64 architecture

  236 |     distArch = 'x86_64'
  237 |   } else {
> 238 |     throw Error(`This action does not support the ${arch} architecture`)
      |           ^
  239 |   }
  240 |
  241 |   return `${distArch}-${distPlatform}`

  at Module.getBaseDist (src/main.ts:238:11)
  at Object.<anonymous> (__tests__/setup.test.ts:79:27)

https://github.com/edgedb/setup-edgedb/blob/main/src/main.ts#L230
https://github.com/edgedb/setup-edgedb/blob/main/__tests__/setup.test.ts#L92

@elprans
Copy link
Member

elprans commented Oct 10, 2023

Oh, feel free to add arm64, normalizing to aarch64, it should work fine

@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 10, 2023

@elprans I think I've fixed the problems. Trigger CI for me?

@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 10, 2023

Thanks, will continue

@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 12, 2023

Everything looks good here. Ready for review/merge when able 😄

Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this!

@elprans elprans merged commit abd93e5 into geldata:main Oct 13, 2023
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.

set-output is deprecated

2 participants