Skip to content

Conversation

@wtrocki
Copy link
Member

@wtrocki wtrocki commented Feb 14, 2025

Proposed changes

CLOUDP-298233

Make transformation to use js. Added nodejs runtime to tools as bonus.

@wtrocki wtrocki requested a review from a team as a code owner February 14, 2025 12:40
@gssbzn
Copy link
Contributor

gssbzn commented Feb 14, 2025

any reason why?

@wtrocki
Copy link
Member Author

wtrocki commented Feb 14, 2025

@gssbzn You mean node versions?

@gssbzn
Copy link
Contributor

gssbzn commented Feb 14, 2025

@gssbzn You mean node versions?

why move from commonjs to module js?

Copy link
Contributor

Choose a reason for hiding this comment

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

node uses commonjs by default should this be .mjs

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I understand it esm is the standard nowadays

Copy link
Member Author

Choose a reason for hiding this comment

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

We have all files as js (which should be fine) using (mjs) import format.
This PR makes single change to align to that format.

If we want to change extension or import format let me know. Can do it across repo comprehensively.

@wtrocki
Copy link
Member Author

wtrocki commented Feb 14, 2025

I introduced it before but we agreed with @lovisaberggren to use esm (js + import format) across all the repository.

@@ -1 +1,2 @@
golang 1.23.0
nodejs 20.9.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looked at the package json reference, we can specify there if we want: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#engines

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That is very common practice. Adding it.

Copy link
Contributor

Choose a reason for hiding this comment

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

adding it to the package.json just produces warnings

@wtrocki wtrocki force-pushed the CLOUDP-298233-postman branch from c1500d5 to 1558240 Compare February 17, 2025 15:10
@wtrocki wtrocki force-pushed the CLOUDP-298233-postman branch from 1558240 to 794fd8a Compare February 17, 2025 15:16
@wtrocki wtrocki merged commit 22f5539 into main Feb 17, 2025
11 checks passed
@wtrocki wtrocki deleted the CLOUDP-298233-postman branch February 17, 2025 17: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.

4 participants