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

breaking: migrate to cjs #163

Merged
merged 3 commits into from
Oct 17, 2023
Merged

breaking: migrate to cjs #163

merged 3 commits into from
Oct 17, 2023

Conversation

Eomm
Copy link
Collaborator

@Eomm Eomm commented Sep 24, 2023

fixes #162

I think after this PR, we could ship the v1

WDYT @mateonunez?

@Eomm
Copy link
Collaborator Author

Eomm commented Sep 24, 2023

TS is failing 💩

@Eomm
Copy link
Collaborator Author

Eomm commented Sep 30, 2023

CC I wrote the TS part with @allevo @lmammino (4 moral support)

@Puppo
Copy link
Contributor

Puppo commented Oct 10, 2023

@Eomm and @mateonunez, I think there is the same problem with the CI here

@mateonunez
Copy link
Owner

Sigh. I wasn't able to reproduce it locally. We need to fix that flaky before v1.0.

Do you have more information about the error, @Puppo? In that case, we can open an issue with all the needed information to reproduce it locally and provide a fix.

@Puppo
Copy link
Contributor

Puppo commented Oct 10, 2023

@mateonunez I don't have any info yet. It seems to be stuck but randomly. I'll look it over.

Copy link
Owner

@mateonunez mateonunez left a comment

Choose a reason for hiding this comment

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

The new typing system rocks, man! Thanks a lot.

I think we can send these improvements to v1.0, we just need to verify why tests sometimes struggle with Node v20.

@@ -1,42 +1,47 @@
import type { FastifyPluginCallback } from 'fastify'
import type { Document, Orama, ProvidedTypes, Results, SearchParams, create } from '@orama/orama'
import type { TypedDocument, insert, Orama, Results, SearchParams, create, AnyOrama, PartialSchemaDeep, Schema } from '@orama/orama'
Copy link
Owner

Choose a reason for hiding this comment

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

Can insert and AnyOrama be removed here? They are no longer used.

@Puppo
Copy link
Contributor

Puppo commented Oct 11, 2023

The new typing system rocks, man! Thanks a lot.

I think we can send these improvements to v1.0, we just need to verify why tests sometimes struggle with Node v20.

@mateonunez, the problem with the test is something relative to the reporter; I'm on it to understand the problem; it seems something rests ongoing and doesn't close the process. But it's random and not easy to debug.

@Puppo
Copy link
Contributor

Puppo commented Oct 11, 2023

I think this is the issue

@mateonunez
Copy link
Owner

I think this is the issue

Nice catch, @Puppo!

Any concerns on this, @Eomm? Should we rollback to node-tap in your opinion?

@Eomm
Copy link
Collaborator Author

Eomm commented Oct 12, 2023

Should we rollback to node-tap in your opinion?

I would wait a sec for a fix before switching.

Tap changed license and I see everybody is migrating to node test runner.

@Puppo
Copy link
Contributor

Puppo commented Oct 17, 2023

Any new here guys?
@mateonunez or @Eomm please, run the CI again meanwhile to check everything is okay.
The error is random so if you try twice or three time you should see the 🟢 light.

@mateonunez
Copy link
Owner

Any new here guys?
@mateonunez or @Eomm please, run the CI again meanwhile to check everything is okay.
The error is random so if you try twice or three time you should see the 🟢 light.

I saw a new version of Node has been released, but don't know if it fixes the issue you attached. I'll read the changelog later. Meanwhile I've re-run the CI.

@Eomm
Copy link
Collaborator Author

Eomm commented Oct 17, 2023

Green

missing approve 😄

Copy link
Owner

@mateonunez mateonunez left a comment

Choose a reason for hiding this comment

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

LGTM

I'll ship the stable version of this plugin tonight.

Thank you so much guys, @Eomm, @Puppo! ♥️

@mateonunez mateonunez merged commit ba735b8 into main Oct 17, 2023
6 checks passed
@mateonunez mateonunez deleted the cjs branch October 17, 2023 16:48
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.

migrate to cjs
3 participants