Skip to content

Conversation

@erunion
Copy link
Member

@erunion erunion commented Sep 12, 2023

🧰 Changes

  • Dropping support for Node 16
  • Fixing issues with our CJS + ESM builds.
    • This required adding .js and .cjs to every file import as well as shifting around how clients and targets extname is setup as our Node snippets are generating snippets with require() and thus need to have .cjs extensions now. Without this work all integration tests for these were failing as you can't use require() in a .js file when type: module is present.
  • Moved unit testing over to Vitest because it sanely handles ESM.

@erunion erunion added the enhancement New feature or request label Sep 12, 2023
@erunion erunion changed the title feat: dropping support for node 16 feat: dropping support for node 16 + esm Sep 13, 2023
@erunion erunion changed the title feat: dropping support for node 16 + esm feat: dropping support for node 16 + esm compat Sep 13, 2023
test(`should return the expected response for \`${fixture}\``, () => {
const basePath = path.join('src', 'targets', targetId, clientId, 'fixtures', `${fixture}${extname(targetId)}`);
const fixtureExtension = extname(targetId, clientId);
const basePath = path.join('src', 'targets', targetId, clientId, 'fixtures', `${fixture}${fixtureExtension}`);

Check warning

Code scanning / CodeQL

Unsafe shell command constructed from library input

This path concatenation which depends on [library input](1) is later used in a [shell command](2). This path concatenation which depends on [library input](1) is later used in a [shell command](3).
Copy link
Member Author

Choose a reason for hiding this comment

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

Marked this as invalid because this is a unit test.

@erunion erunion added the refactor Issues about tackling technical debt label Sep 13, 2023
@erunion erunion requested a review from kanadgupta September 13, 2023 01:18
context: .
dockerfile: integrations/python.Dockerfile
command: 'npx jest src/integration.test.ts'
command: 'npx vitest run src/integration.test.ts'
Copy link
Member Author

Choose a reason for hiding this comment

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

Have to use the run command here because Vitest doesn't know in this Docker container that it's being run in a CI environment and it'll sit there in watch mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Had to rename these all to be .cjs because they the snippet that's being generated here uses require().

@erunion erunion merged commit 487030f into main Sep 14, 2023
@erunion erunion deleted the feat/drop-node16 branch September 14, 2023 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor Issues about tackling technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants