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

Convert to TypeScript #66

Merged
merged 6 commits into from
Mar 8, 2023
Merged

Convert to TypeScript #66

merged 6 commits into from
Mar 8, 2023

Conversation

bobbyiliev
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 👷 Optimization
  • 📝 Documentation Update
  • 🚩 Other

Description

Initial conversation to TS.

Related Tickets & Documents

Closes #60

Added to documentation?

  • 📜 readme
  • 🙅 no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

@sjwiesman
Copy link
Contributor

Wow you are fast. Why is this marked in progress? We don't need to make everything idiomatic in this PR. Getting the build system updated is most important. Everything else can be piecemeal.

@sjwiesman
Copy link
Contributor

If this compiles and works, I'm inclined to say merge and increment.

@bobbyiliev bobbyiliev force-pushed the typescript branch 2 times, most recently from 20225a8 to 47ebce3 Compare March 7, 2023 20:29
@bobbyiliev
Copy link
Collaborator Author

Wow you are fast. Why is this marked in progress? We don't need to make everything idiomatic in this PR. Getting the build system updated is most important. Everything else can be piecemeal.

There is one bug that the integration tests caught:

| ✔  Schema registered!  Subject: mz_datagen_comments-value, ID: 4
| 
| /app/node_modules/@kafkajs/confluent-schema-registry/dist/SchemaRegistry.js:153
|             throw new errors_1.ConfluentSchemaRegistryArgumentError(`Invalid registryId: ${JSON.stringify(registryId)}`);
|                   ^
| 
| ConfluentSchemaRegistryArgumentError: Invalid registryId: undefined
|     at SchemaRegistry.encode (/app/node_modules/@kafkajs/confluent-schema-registry/dist/SchemaRegistry.js:153:19)
|     at file:///app/dist/src/schemas/schemaRegistry.js:65:44

Wanted to fix that first before merging

@chuck-alt-delete
Copy link
Contributor

chuck-alt-delete commented Mar 7, 2023

That bug (I think) and a couple of others are fixed in my open PR. Maybe we can merge that one first?

@bobbyiliev
Copy link
Collaborator Author

That bug (I think) and a couple of others are fixed in my open PR. Maybe we can merge that one first?

Ok sounds good, let's merge #62 and then I'll rebase and go from there!

@chuck-alt-delete
Copy link
Contributor

Ok done!

@bobbyiliev
Copy link
Collaborator Author

Just pushed the changes from main. Seems like we are still hitting that error:

| ✔  Registering Avro schema...  
| 
| file:///app/dist/src/schemas/schemaRegistry.js:17
|         schema.name = `name${i++}`;
|                     ^
| 
| TypeError: Cannot create property 'name' on string 'string'

@chuck-alt-delete
Copy link
Contributor

That looks like a new error

@bobbyiliev
Copy link
Collaborator Author

That looks like a new error

Ah yes, no that was actually the initial error, before I started debugging and trying out stuff

@bobbyiliev bobbyiliev marked this pull request as ready for review March 7, 2023 22:38
@bobbyiliev bobbyiliev merged commit 558e308 into main Mar 8, 2023
@bobbyiliev bobbyiliev deleted the typescript branch March 8, 2023 09:07
recursethis pushed a commit to recursethis/materialize-datagen that referenced this pull request Feb 26, 2024
* Convert to TypeScript

* Allow integration tests on PRs

* Update avro name hook

* Fix CSR prefix bug
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.

Feature: Consider migrating to typescript
3 participants