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.

Sign up for GitHub

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

Jump to bottom

Deprecate auto location #19510

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Deprecate auto location #19510

merged 3 commits into from
Nov 5, 2021

Conversation

sandstrom
Copy link
Contributor

@sandstrom sandstrom commented Apr 27, 2021 •
edited
Loading

Code to implement the Deprecate Auto Location RFC.

Open Questions

  • What's the strategy around failing tests? Right now, it seems like tests are failing due to the deprecation messages itself. What's the recommended approach there?

TODO

  • Modify deprecation messages (if needed?)
  • Add an entry into the Ember deprecation table (guides repo?)

Related PRs

@rwjblue I know you have a bunch of stuff on your plate, but if you have time, feel free to have a look at this.

@Windvis
Copy link
Contributor

Windvis commented Apr 30, 2021

@sandstrom I think you can use the expectDeprecation test helper, that would also test if the deprecations are working correctly 😃.

@rwjblue
Copy link
Member

rwjblue commented May 24, 2021

This looks good overall. I think once we have some tests, we should be good to go.

@sandstrom sandstrom force-pushed the deprecate-auto-location branch 4 times, most recently from b660c09 to 7205240 Compare May 25, 2021 14:59
@sandstrom
Copy link
Contributor Author

sandstrom commented May 25, 2021 •
edited
Loading

@rwjblue I've gotten the tests running.

There is a failure in the "Browserstack Tests", but that error seems unrelated (other PRs the past few days in this repo have the same issue).

I haven't added any tests checking for the actual deprecation messages though, don't know if that's required.

Hopefully we can get this in before the 4.0 release.

not ok 7 IE 11.0 - [98 ms] - Components test: <Input @type='checkbox' />:  sends an action with `<Input EVENT={{action "foo"}} />` for native DOM events
    ---
        actual: >
            touchstart,touchmove,touchend,touchcancel,keydown,keyup,keypress,mousedown,mouseup,contextmenu,change,click,dblclick,submit,input,change,dragstart,drag,dragenter,dragleave,dragover,drop,dragend,focusin
        expected: >
            touchstart,touchmove,touchend,touchcancel,keydown,keyup,keypress,mousedown,mouseup,contextmenu,change,click,dblclick,submit,input,change,dragstart,drag,dragenter,dragleave,dragover,drop,dragend
        stack: >
               at assertTriggersNativeDOMEvents (http://127.0.0.1:7774/196963099088/dist/tests/ember-tests.js:19111:7)
               at testSendsAnActionWithInputEVENTActionFooForNativeDOMEvents (http://127.0.0.1:7774/196963099088/dist/tests/ember-tests.js:20064:7)
               at Anonymous function (http://127.0.0.1:7774/196963099088/dist/tests/ember-tests.js:121641:11)
               at runTest (http://127.0.0.1:7774/196963099088/dist/tests/qunit/qunit.js:2251:8)
               at run (http://127.0.0.1:7774/196963099088/dist/tests/qunit/qunit.js:2239:8)
               at Anonymous function (http://127.0.0.1:7774/196963099088/dist/tests/qunit/qunit.js:2461:10)
        message: >
            called for all events
        negative: >
            false
        browser log: |
    ...

@sandstrom sandstrom mentioned this pull request Jun 17, 2021
Merged
This was referenced Jun 28, 2021
Closed
Closed
Closed
mixonic
mixonic requested changes Jul 17, 2021
View reviewed changes
Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

@sandstrom I'm sorry we didn't manage to get this landed before 4.0. It seems like it was pretty close, and then summer hours hit and this fell off our radar. I really appreciate the effort you put in.

Can you tweak this and the deprecation guide PR to target 5.0?

@mixonic mixonic mentioned this pull request Jul 18, 2021
Closed
19 tasks
@rwjblue rwjblue marked this pull request as draft July 21, 2021 16:00
@sandstrom
Deprecate auto location
cec2980
@sandstrom
Deprecate calling Location.create
5af5b9f 
This functionality has been deprecated since 2014,
but there wasn't any deprecation message before.

See commit #1d1d0db4 for details.
@sandstrom
Deprecate Location.detect
f74ef2a 
We're only showing the deprecation warning for locations other
than 'auto', since auto already have it's own deprecation message.

We're also moving the rootURL trailing slash check from detect
to initState. That assert has nothing to do with detection, but it
was likely added there because that method is called *after*
the rootURL value is set on the location (by the router).

Luckily, the initState method is also called after that value is set,
so we can use that hook instead.

We're hijacking a method for something that isn't it's primary purpose,
but that was the case with the existing code too, so this seems
like a reasonable solution.
@sandstrom
Copy link
Contributor Author

@mixonic I've updated the PR to target 5.0.0. I set the since/enabled value to 4.1.0, since I think 4.0.0 may be released soon, and possibly before anyone has had time to merge this.

But let me know if you want to drop this down to 4.0.0 for the since/enabled value.

@sandstrom sandstrom marked this pull request as ready for review October 13, 2021 14:49
@sandstrom
Copy link
Contributor Author

sandstrom commented Oct 17, 2021 •
edited
Loading

friendly ping @mixonic 😄

@rwjblue rwjblue merged commit 90b5d20 into emberjs:master Nov 5, 2021
@sandstrom sandstrom deleted the deprecate-auto-location branch November 8, 2021 09:30
@jaredgalanis jaredgalanis mentioned this pull request Dec 30, 2021
Merged
6 tasks
@sandstrom sandstrom mentioned this pull request Feb 23, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@mixonic mixonic Awaiting requested review from mixonic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sandstrom @Windvis @rwjblue @mixonic