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

teraslice esm migration #3511

Merged
merged 21 commits into from
Jan 23, 2024
Merged

teraslice esm migration #3511

merged 21 commits into from
Jan 23, 2024

Conversation

jsnoble
Copy link
Member

@jsnoble jsnoble commented Dec 22, 2023

  • migrate teraslice to use esm modules natively

Issues:

  • eslint needs to be updated to be able to correctly parse typescripte >5.2. It throws warning
  • eslint is having a problem linting an esm module based package. The primary issue is that this is a monorepo with several packages and we are slowly converting code over. For now I am ignoring linting on teraslice until I can figure out a way to do it independently for each package, assuming its worth the dev effort.
  • using @swc/jest for typescript compiling for jest testing is causing errors with class decorators, so I am keeping ts-jest for the rest of the tests and using the new one until it can be properly configured

Copy link
Member

@godber godber left a comment

Choose a reason for hiding this comment

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

I have some questions here.


const config = module.default(dirPath);

config.preset = "ts-jest/presets/default-esm";
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we get rid of ts-jest, is this config still necessary?

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 are still using ts-jest for almost the rest of the packages so its not removed, going to do this library by library and not mess with things that work untill we can remove it all after conversion.

@@ -1,14 +1,3 @@
// TODO: see if I can fix this
function hasKafkaConnector() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this was originally added?

Copy link
Member Author

Choose a reason for hiding this comment

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

to conditionally add default configuration if the library is being used, I just made it so its always there

this.gcStats = require('gc-stats')();
// @ts-expect-error
const stats = await import('gc-stats');
console.log('stats', stats);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log('stats', stats);

Do we still want this console.log?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a good catch, looking into it

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the only thing you need to actually resolve on this PR prior to merge.

Please let me know when you're done and want review again.

@godber godber merged commit c1358e8 into master Jan 23, 2024
39 checks passed
@godber godber deleted the teraslice-esm branch January 23, 2024 21:06
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.

2 participants