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

Meteor 3.0 migration #444

Merged
merged 41 commits into from
Jan 18, 2024
Merged

Meteor 3.0 migration #444

merged 41 commits into from
Jan 18, 2024

Conversation

harryadel
Copy link
Member

@harryadel harryadel commented Dec 20, 2023

A continuation of @klablink work

You can test the changes locally by updating the package version to 4.0.0-beta.7

@harryadel
Copy link
Member Author

harryadel commented Dec 24, 2023

@StorytellerCZ @jankapunkt @klablink The test app got updated to 3.0-alpha.19 and the tests are passing. The meteortesting:mocha in use now is 3.0.3-alpha300.11 thanks to @Grubba27. Good call on you to mention your work in the excel file. I'd not have known otherwise.

@jankapunkt
Copy link
Member

Perfect, a beta would be awesome

@harryadel
Copy link
Member Author

coming up...

@harryadel
Copy link
Member Author

harryadel commented Dec 26, 2023

A new version 4.0.0-beta.4 is published where the helper code made for setting sync or async DB calls got reworked.

cc @StorytellerCZ @jankapunkt @klablink

@alisnic
Copy link

alisnic commented Dec 27, 2023

A new version 4.0.0-beta.4 is published

2400 unit tests and 821 e2e tests pass in our product with this release 🎉 The only thing we needed to do is to remove schema-index package

@harryadel
Copy link
Member Author

Ah finally someone reported in 🎉, thank you @alisnic.

Did you run into any problems? Did you have to go through any extra steps to migrate?

I was thinking of releasing a new beta where we're no longer reliant on the NPM version of simple-schema and instead use @jankapunkt fork. What do you think?

@xet7
Copy link

xet7 commented Dec 30, 2023

@harryadel

Currently I'm using these. meteor update breaks it.

https://github.com/wekan/wekan/blob/main/.meteor/versions#L4-L8

@xet7
Copy link

xet7 commented Dec 30, 2023

@harryadel

I have also tried this Meteor 3 PR, forking it to wekan/packages, but it does not work with Meteor 2.14.

@xet7
Copy link

xet7 commented Dec 30, 2023

@harryadel

After doing meteor update, I also tried with npm remove simpl-schema, but I still get error SimpleSchema is not defined.

@harryadel
Copy link
Member Author

Thanks for reporting it, @xet7. I'll look it into it.

@StorytellerCZ
Copy link
Member

StorytellerCZ commented Dec 30, 2023

@xet7 at the first look, the versions are a bit of a mess. You are using v2 of collection hook, while schema-deny and schema-index are at v1, you will first need to manually update to v3 and use the npm version of simple-schema. Once that is aligned the error should go away. As far as I can see it has nothing to do with this PR and the v4 betas.

Update: @xet7 I have a PR for you soon.

@harryadel
Copy link
Member Author

harryadel commented Jan 8, 2024

4.0.0-beta.7 solves a issue when using collection2 with quave:synced-cron. quave:synced-cron checks for the error code if an entry fails and returns/fails silently if it's a duplicate entry. This works fine when synced-cron is used on its own but when paired with collection2 which modifies Meteor's collection internals and wraps up errors, this becomes a problem. So, now collection2 passes the error code when throwing out an error.

https://github.com/quavedev/meteor-synced-cron/blob/7212459895924a866f8e45e98026164d5f43fd15/synced-cron-server.js#L221

I've a fork of quave:synced-cron that validates this problem where I overhauled the tests and the CI fails as aldeed:collection2@4.0.0-beta.6 is in use. But I'm not able to create PR to merge it, why tho? 🤔 cc @filipenevola

@jankapunkt
Copy link
Member

@harryadel I will update tests on lai:collection-extensions so they're using beta.7 and I'll let you know asap if things are working

@jankapunkt
Copy link
Member

After updating to beta 7 I can't run tests anymore:

=> Started proxy.                             
/home/jankapunkt/.meteor/packages/meteor-tool/.3.0.0-beta.0.ag3n3e.4pp4++os.linux.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.linux.x86_64/tools/runners/run-all.js:170
          Console.log("Error starting Mongo (".concat(left, " left): ").concat(error.message));
                  ^

[TypeError: Console.log is not a function
  at /home/jankapunkt/.meteor/packages/meteor-tool/.3.0.0-beta.0.ag3n3e.4pp4++os.linux.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.linux.x86_64/tools/runners/run-all.js:170:19
  at runNextTicks (node:internal/process/task_queues:60:5)
  at listOnTimeout (node:internal/timers:540:9)
  at processTimers (node:internal/timers:514:7)
]

@harryadel
Copy link
Member Author

Will you please push your latest changes here so I can test things out?

@filipenevola
Copy link

But I'm not able to create PR to merge it

What is happening? We don't have any unique settings in this repo, not on purpose, at least.

Feel free to reach me in pvt as well if you need help sending the PR. Contributions are always welcome.

@DanielDornhardt
Copy link

Console.log("Error starting Mongo (".concat(left, " left): ").concat(error.message));

Sorry if this is super besides the point, I just want to mention that I remember console being spelled with a lowercase "c" instead of an uppercase C. Although I guess the real question is probably another, I just wanted to add a drive-by comment...

@minhna
Copy link

minhna commented Jan 12, 2024

I tried and it works. The app works.
I have a problem when run meteor test --driver-package meteortesting:mocha
Error: TypeError: Settings.attachSchema is not a function
Settings is one of my collection.
Then I need to add this import to the .test file:

import "meteor/aldeed:collection2/static";

@harryadel
Copy link
Member Author

@minhna There're two breaking changes first the replacement of NPM simpl-schema where we now use @jankapunkt fork and static/dynamic loading. But otherwise it's perfectly compatible.

@minhna
Copy link

minhna commented Jan 12, 2024

Do we have another way to load collection2 in test mode? I added that import to one of my test files (which reported error) and it works with other files but I feel it's not the right way doing it.

@harryadel
Copy link
Member Author

You can also dynamically load it

841a7b3

@harryadel
Copy link
Member Author

4.0.0 is released 🎉 , thank you guys!

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.

None yet

8 participants