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

Updated ember required and ember validators packages #715

Conversation

kprasadpvv
Copy link

Resolves #

  • To get the updated version of ember-cli-babel which is causing depreciation warnings

Screenshot 2022-03-02 at 4 23 09 PM

Changes proposed:

  • Updating ember-validators as it's current version is already having updated babel version

Tasks:

  • Added test case(s)
  • Updated documentation

@kprasadpvv
Copy link
Author

@fsmanuel could you please review the PR

@fsmanuel fsmanuel mentioned this pull request Jul 7, 2022
14 tasks
Copy link
Contributor

@fsmanuel fsmanuel left a comment

Choose a reason for hiding this comment

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

@kprasadpvv Looks good to me but I have some concerns how we release it. I opend a issue to discuss this: #716
Happy to get your input.

"ember-require-module": "^0.3.0",
"ember-validators": "^3.0.1"
"ember-require-module": "^0.4.0",
"ember-validators": "^4.1.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to include the breaking changes in the changelog. See my review comment.

Copy link
Author

@kprasadpvv kprasadpvv Jul 8, 2022

Choose a reason for hiding this comment

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

Hi @fsmanuel,
can we release these dependencies update as v4.0.0-beta.13 and release 4.0 after addressing #714
(or)
These dependency changes should also be done as a part of release 4.0 ?

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 we should use the fact that the addon is in beta so we can add the breaking changes in a 4.0 release. I hope it's not too painful. Can you rebase and add a 4.0.0 entry to the changelog and add the following changes:

Breaking changes from ember-validators

  • removed custom String 'now' argument.
  • remove momentjs
  • Remove precision argument. If you need to compare based on precision, you can use the Intl.DateTimeFormat APIs to hone in on the comparison - { year: 'numeric' }
  • Added locale option. Defaults to en-us when creating date times using Intl.DateTimeFormat API.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @fsmanuel , I've update as per the above comment.
Please have a look and let me know if anything needs to be done.

@fsmanuel
Copy link
Contributor

@kprasadpvv kprasadpvv force-pushed the updateValidatorsAndRequiredModule branch from bf1481a to 026643a Compare July 11, 2022 13:32
@kprasadpvv
Copy link
Author

kprasadpvv commented Jul 11, 2022

@NullVoxPopuli would you be so kind and approve the github actions: https://github.com/adopted-ember-addons/ember-cp-validations/actions/runs/2621043171

@NullVoxPopuli , could you please approve again for git actions to run as I fixed the failing test cases. Thank you in advance.

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Approach:

The idea is to have a new beta release for ember versions < 4.0 so the tests for ember-4.0, release, beta, and canary are expected to fail. After the beta release we can start to make it work for ember 4.0 (remove old/private imports, etc)

The relevant tests for this approach pass, so I'm approving this PR.

@MelSumner MelSumner merged commit 1989855 into adopted-ember-addons:master Aug 17, 2022
@fsmanuel
Copy link
Contributor

@MelSumner Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants