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

feat: update @comapeo/schema to 1.2.0, adding alerts and server #805

Merged
merged 27 commits into from
Oct 24, 2024

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Sep 2, 2024

this installs a local version of mapeo-schema with remoteDetectionAlert schema.

should close #806

Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM once we (1) release a new @mapeo/schema (2) fix the type errors.

Could also be good to add a test that runs a remoteDetectionAlert through the whole system to make sure it works. Maybe just create one and make sure we can read it.

@tomasciccola
Copy link
Contributor Author

I'm getting a type error on the test-e2e/project-crud.js, where I added a fixture for remoteDetectionAlert. I don't know why it expects its field metadata to have a manualLocation. Its basically confusing it with an observation. I can add that field to please the type checker since metadata in remoteDetectionAlert allows for any value for any string key, but it feels finicky.

On the other hand, I can't npm i now, because mapeo/schema is a peerDependency in mock-data so the local version installed in this PR doesn't match the version on mock-data...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's time for us to make a new migration!

I think we plan to add this to a version of @mapeo/core that the MVP doesn't depend on--something like v1.1.0. Given that, I think we'll need a new migration here.

@EvanHahn
Copy link
Contributor

Now that we've released @comapeo/[email protected], I think we can revisit this.

@tomasciccola
Copy link
Contributor Author

tomasciccola commented Sep 26, 2024

@EvanHahn I'm having a couple of type errors on test-e2e/project-crud.js which I'm not sure how to interpret...

Also, the awana-lockfile-bot is showing a package-lock change with @mapeo/schema. I think we still have transitive dependencies on mock-data to mapeo/schema, could the type error be related to that (I don't think though...)

@tomasciccola
Copy link
Contributor Author

I think something related to ts's structural typing is tripping us here. Since 'observation' and 'remoteDetectionAlert' both have a metadata field I think it expects remoteDetectionAlert.metadata to be the same as observation.metadata?
e.g. adding manualLocation to remoteDetectionAlert fixes all type errors... @EvanHahn any clue?

EvanHahn added a commit that referenced this pull request Sep 30, 2024
Our project CRUD tests had a number of `ts-ignore`s and `any`s. This
replaces those with type-safe code.

I think this is a useful change on its own but will also make [an
upcoming change][0] easier.

[0]: #805
EvanHahn added a commit that referenced this pull request Oct 1, 2024
Our project CRUD tests had a number of `ts-ignore`s and `any`s. This
replaces those with type-safe code.

I think this is a useful change on its own but will also make [an
upcoming change][0] easier.

[0]: #805
@tomasciccola
Copy link
Contributor Author

tomasciccola commented Oct 1, 2024

So, after this PR was merged I updated the branch but I still get what is basically the same type error:
We have two schemas with a field with the same name ({observation,remoteDetectionAlert}.metadata).
It expects remoteDetectionAlert.metadata to have manualLocation which actuall belongs to observation.
I think I integrated the changes right (basically adding remoteDetectionAlert as a case in create, createWithMockData, update and getUpdatedFixture) but the issue still persists :'(

Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

This still needs a new migration (rather than deleting the old one), but I think is ready to go otherwise.

@awana-lockfile-bot
Copy link

package-lock.json changes

Click to toggle table visibility
Name Status Previous Current
@comapeo/geometry ADDED - 1.0.2
@comapeo/schema UPDATED 1.0.0 1.1.1
@mapeo/mock-data UPDATED 2.0.0 2.1.1
dereference-json-schema ADDED - 0.2.1
protobufjs UPDATED 7.2.5 7.4.0

@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@digidem digidem deleted a comment from awana-lockfile-bot bot Oct 24, 2024
@awana-lockfile-bot
Copy link

awana-lockfile-bot bot commented Oct 24, 2024

package-lock.json changes

Click to toggle table visibility
Name Status Previous Current
@comapeo/geometry ADDED - 1.0.2
@comapeo/schema UPDATED 1.0.0 1.2.0
@mapeo/mock-data UPDATED 2.0.0 2.1.1
dereference-json-schema ADDED - 0.2.1
protobufjs UPDATED 7.2.5 7.4.0

@EvanHahn EvanHahn assigned EvanHahn and unassigned tomasciccola Oct 24, 2024
@EvanHahn EvanHahn merged commit 98d9981 into main Oct 24, 2024
8 checks passed
@EvanHahn EvanHahn deleted the feat/addRemoteDetectionAlertDataType branch October 24, 2024 15:52
@EvanHahn EvanHahn changed the title feat: add remoteDetectionAlert dataType feat: update @comapeo/schema to 1.2.0, adding alerts and server Oct 24, 2024
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.

Add alerts data type
2 participants