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

Avoid the Ember global deprecation #118

Merged
merged 2 commits into from
Jul 4, 2021
Merged

Conversation

mixonic
Copy link
Owner

@mixonic mixonic commented May 28, 2021

Following a suggestion at #107 (comment), use the self.require API to avoid using the Ember global when possible.

However there are other uses of deprecated Ember in this addon as well. These are found in the test suite. Make the test suite a bit more defensive and update deprecated API uses.

Summary:

TODO:

  • Drop the template compiler deprecation catching. This was broken by an upstream change, and @rwjblue and I discussed a different path to move forward. However since this behavior is already broken for most apps, we should just drop it and ship 2.0 the add it back in a later patch/release.

@mixonic mixonic mentioned this pull request May 28, 2021
2 tasks
@mixonic mixonic added 2.0 2.0 blockers, see https://github.com/mixonic/ember-cli-deprecation-workflow/issues/109 bug labels May 28, 2021
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch 2 times, most recently from e4d58b1 to 184d80c Compare May 28, 2021 03:23
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch 2 times, most recently from b67a051 to c9e2029 Compare May 28, 2021 12:31
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch 5 times, most recently from 37ae768 to fd95838 Compare June 4, 2021 01:23
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch from fd95838 to 9c9ddcc Compare June 4, 2021 01:55
@mixonic mixonic changed the title Avoid several Ember API deprecations Avoid the Ember global deprecation Jun 4, 2021
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch 2 times, most recently from b4b4b76 to 1701ee2 Compare June 4, 2021 02:32
* Avoid the Ember global in workflow runtime code.

This patch is dependent on
#125 which
included the code at emberjs/ember-jquery#321
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch from 1701ee2 to f6cd0c4 Compare June 12, 2021 15:37
This was broken long ago by a change in ember-cli-htmlbars. There is
a plan to bring it back, but remove it for now since the codepath is not
doing anything for most codebases.
@mixonic mixonic force-pushed the mixonic/requirejs-ember-access branch from c4300b1 to 6fa5784 Compare July 3, 2021 12:53
@mixonic mixonic marked this pull request as ready for review July 3, 2021 12:59
@mixonic mixonic requested a review from rwjblue July 3, 2021 12:59
@mixonic mixonic mentioned this pull request Jul 3, 2021
@mixonic mixonic merged commit 23e8017 into master Jul 4, 2021
@mixonic mixonic deleted the mixonic/requirejs-ember-access branch July 4, 2021 00:27
@mixonic
Copy link
Owner Author

mixonic commented Jul 4, 2021

This is included in https://github.com/mixonic/ember-cli-deprecation-workflow/releases/tag/v2.0.0-beta.5, please open issues if you have any trouble with it.

@boris-petrov
Copy link

@mixonic - thanks for trying to fix the deprecation warnings! However, this broke my app: Uncaught TypeError: require.has is not a function. The line is this one. Any ideas?

@mixonic
Copy link
Owner Author

mixonic commented Jul 5, 2021

@boris-petrov what version of Ember CLI, Ember, Node?

@boris-petrov
Copy link

Everything is the latest version - Ember CLI 3.27.0, Ember 3.27.5, Node 16.4.1.

@mixonic
Copy link
Owner Author

mixonic commented Jul 5, 2021

@boris-petrov Loader.js version? Are you using Embroider?

@boris-petrov
Copy link

As I said, everything is the latest version. 😄 Loader.js - 4.7.0. Not using Embroider, no.

Is require.has supposed to be there? require is but has is not...

@mixonic
Copy link
Owner Author

mixonic commented Jul 5, 2021

Yeah it is expected as part of the loader. I mean, clearly it is expected, right? It is tested here and I additionally booted an app to check these changes. I'll try to reproduce the issue with exactly the versions you mentioned. In the back of my head 'has' being missing sounds like something I have run into before, but I'll need to dive into the loader and some stuff.

Thanks for raising it, sounds like something we need to address quickly.

@boris-petrov
Copy link

Thank you for taking the time! Please tell me if you can't reproduce it - it might be something specific to my case (although I doubt that) - so in that case I'll try to figure out what's going on.

@mixonic
Copy link
Owner Author

mixonic commented Jul 5, 2021

@boris-petrov ok, I set an app to those versions and to the 2.0.0 release of workflow, and no luck reproducing.

has should be defined on window.require, I believe via this line in loader.js: https://github.com/ember-cli/loader.js/blob/master/lib/loader/loader.js#L332 (it indirectly sets it on requirejs but that variables refers to the same value (a function) as require.

@mixonic
Copy link
Owner Author

mixonic commented Jul 5, 2021

@boris-petrov if you drop a debugger onto the line where the exception is firing from, and add one inside loader.js, you might be able to rationalize what order things are happening in and if the loader is being set up as expected.

@boris-petrov
Copy link

@mixonic - thanks for trying it out. I should have debugged it before asking here. The problem is, as always, in ember-classic-decorator. I'll open an issue there. Thank you for the time and sorry for the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 2.0 blockers, see https://github.com/mixonic/ember-cli-deprecation-workflow/issues/109 bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants