-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
Migration to meteor 3.0 #1737
Migration to meteor 3.0 #1737
Conversation
…ckages/meteor-autoform into migration/meteor-3.0
…utoform into migration/meteor-3.0
…utoform into migration/meteor-3.0
published |
If there are no deep issues with this release then I will publish 8.0.0 next week |
When is this PR getting merged? The comment said next week, and it has been a month. |
@TMeerhof sorry too much on my plate this autumn. Unfortunately nobody made a code-review and I personally did not tests against all my apps due my current crunch. Anyone who can help out with a code review o feedback on RC.4? // @harryadel @ricaragao @lynchem @polygonwood @manueltimita |
Jan
I have limited experience in code reviews, I can give it a try. Can you
point me to the pull request ? I see 3 pull requests in meteor-autoform on
Github, is it one of those 3 ? Or is this on another fork ?
regards
Polygonwood
…On Mon, Nov 4, 2024 at 5:05 PM Jan Küster ***@***.***> wrote:
@TMeerhof <https://github.com/TMeerhof> sorry too much on my plate this
autumn. Unfortunately nobody made a code-review and I personally did not
tests against all my apps due my current crunch.
Anyone who can help out with a code review o feedback on RC.4? //
@harryadel <https://github.com/harryadel> @ricaragao
<https://github.com/ricaragao> @lynchem <https://github.com/lynchem>
@polygonwood <https://github.com/polygonwood> @manueltimita
<https://github.com/manueltimita>
—
Reply to this email directly, view it on GitHub
<#1737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMTJC25KT5MKRFRDHHV7LLTZ66LOPAVCNFSM6AAAAABA3A5IWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGEYDQMBWHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
@polygonwood Thank you for participating! It's exactly this PR you just have just commented. The files changed tab allows you to view all code changes of this PR and you can add comments (start review) to respective lines and even suggest changes. Your comments through review will be pending until you submit the review so you can freely add or remove them before they get attached to this pull request. The GitHub docs are also very helpful on the topic. |
OK I do have some time available Thu/Fri this week, I will have look at it
(new experience ...)
regards
Ronny
…On Mon, Nov 4, 2024 at 7:05 PM Jan Küster ***@***.***> wrote:
@polygonwood <https://github.com/polygonwood> Thank you for
participating! It's exactly this PR you just have just commented. The files
changed tab
<https://github.com/Meteor-Community-Packages/meteor-autoform/pull/1737/files>
allows you to view all code changes of this PR and you can add comments
(start review) to respective lines and even suggest changes.
Your comments through review will be pending until you submit the review
so you can freely add or remove them before they get attached to this pull
request. The GitHub docs are also very helpful on the topic.
—
Reply to this email directly, view it on GitHub
<#1737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMTJC2YMDEZ5BXVYY4S7E73Z66ZNZAVCNFSM6AAAAABA3A5IWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGM3TKNBVGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
getMoment.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need moment ? javascript date functions are not sufficient ? If not, is moment the better choice versus e.g. luxon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we stick with moment for backwards compatibility but its a weak dependency and should 100% work without moment being installed and it should also not cause moment being added to .meteor/versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jan. It's best to keep it as is for now and update things in a later bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed as an AutoForm user. Missing is some form of documentation of what has been changed and what were the drivers to do so. Changes look OK and properly coded.
Jan
sorry I had some delay but I submitted my review. Not easy for a 'user' to
review the internals :-)
In my app we use Autoform quite extensively, we go live this weekend with
M3 version, I have not encountered any difficulties.
Once this version 8.x.x is official, I will add some pull requests on some
extensions/modifications we have added (and which are now in local
packages).
regards
Polygonwood
…On Mon, Nov 4, 2024 at 7:05 PM Jan Küster ***@***.***> wrote:
@polygonwood <https://github.com/polygonwood> Thank you for
participating! It's exactly this PR you just have just commented. The files
changed tab
<https://github.com/Meteor-Community-Packages/meteor-autoform/pull/1737/files>
allows you to view all code changes of this PR and you can add comments
(start review) to respective lines and even suggest changes.
Your comments through review will be pending until you submit the review
so you can freely add or remove them before they get attached to this pull
request. The GitHub docs are also very helpful on the topic.
—
Reply to this email directly, view it on GitHub
<#1737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMTJC2YMDEZ5BXVYY4S7E73Z66ZNZAVCNFSM6AAAAABA3A5IWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGM3TKNBVGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
Yes, I can @jankapunkt . I'm starting to migrate some Meteor Apps that use this package, so I can review and test the code. Did you publish this RC version to meteor package? Or should I clone the repository?
|
@ricaragao yes the 8.0.0 RC releases are available as Meteor packages |
@polygonwood thanks a lot for the testing and review! If I understand you correctly then you would want some form of changelog for the 8.0.0 release, correct? I can try to add it and update the README in general, I think it contains some outdated links. |
Jan
changelog is for sure interesting for those that are migrating, but as a
(first time) reviewer what would be interesting is a (short) description of
what is being changed and why. E.g. I noticed the change in the
ArrayTracker, I can guess it's because of Async issues - or Blaze changes,
but I also noticed changes from 'function' to 'class' (which is a good
change of course), but was this driven by 3.0 migration or just a
beautification ?
regards
Polygonwood
…On Thu, Nov 21, 2024 at 9:08 PM Jan Küster ***@***.***> wrote:
@polygonwood <https://github.com/polygonwood> thanks a lot for the
testing and review! If I understand you correctly then you would want some
form of changelog for the 8.0.0 release, correct?
I can try to add it and update the README in general, I think it contains
some outdated links.
—
Reply to this email directly, view it on GitHub
<#1737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMTJC22X7QLGGCPIXC6IRS32BY4SJAVCNFSM6AAAAABA3A5IWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJSGE3DIMBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
.github/workflows/testsuite.yml
Outdated
|
||
- name: Setup meteor | ||
uses: meteorengineer/setup-meteor@v1 | ||
with: | ||
meteor-release: '1.10.1' | ||
meteor-release: '3.0-rc.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a version bump to the latest version 3.1
@@ -37,49 +36,7 @@ Any code change should be submitted as a pull request. The description should ex | |||
The bigger the pull request, the longer it will take to review and merge. Try to break down large pull requests in smaller chunks that are easier to review and merge. | |||
It is also always helpful to have some context for your pull request. What was the purpose? Why does it matter to you? | |||
|
|||
## Financial contributions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bring it back and link to yourself and/or Czech Jan. You're doing great work and should be compensated if possible.
const self = this | ||
self.info = {} | ||
} | ||
export class ArrayTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job using Class syntax 👍
|
||
Package.onUse(function (api) { | ||
api.versionsFrom('[email protected]') | ||
api.versionsFrom(['3.0.1']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to keep versionsFrom
in sync with CI meteor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did take a look around, there doesn't seem to be any major changes like I had feared. I'd recommend doing a beta release so others can test things out.
@jankapunkt Is there anything holding this PR back from merging? |
Published |
This is an ongoing PR. With every update this description will extend.
Related migration for the themes: Meteor-Community-Packages/meteor-autoform-themes#10