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

Deprecate window global #19357

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

No description provided.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I think this will also need a couple of tests

packages/ember/index.js Outdated Show resolved Hide resolved
packages/ember/index.js Outdated Show resolved Hide resolved
packages/ember/index.js Outdated Show resolved Hide resolved
packages/ember/index.js Outdated Show resolved Hide resolved
packages/ember/index.js Outdated Show resolved Hide resolved
@pzuraq
Copy link
Contributor

pzuraq commented Jan 26, 2021

So had a quick convo with @rwjblue about this, and we realized we're going to need to make some additional changes. Currently, ember-cli-babel still transpiles import * from '@ember/*' to use the Ember global, and it does not add import Ember from 'ember'; at the top. In addition, we don't actually define an ember module - that is also transpiled via ember-cli-babel.

So, in this package we need to add the Ember package so you can actually import it. In the babel plugin that ember-cli-babel adds, we need to update it to use import Ember from 'ember'; to the top. It should also only do that IF the Ember version is high enough, so probably 3.26.0-beta.1 assuming we get this PR in before then.

@NullVoxPopuli NullVoxPopuli force-pushed the deprecate-window-global branch from e0fdb0f to bc14af2 Compare January 27, 2021 20:24
@NullVoxPopuli NullVoxPopuli changed the title WIP: Deprecate window global Deprecate window global Jan 27, 2021
@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jan 27, 2021

Next Steps?:

  • update ember-cli-babel
  • add deprecation guide
  • add tests for this deprecation in both ember.js and ember-cli-babel
  • merge this PR?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

In order to land we need to:

  • Update https://github.com/ember-cli/babel-plugin-ember-modules-api-polyfill
    • Add a new configuration option (useEmberModule maybe?) to opt in to emitting the import Ember from 'ember'; bit or doing the (currently default) "just use window.Ember" thing
  • Update ember-cli-babel
    • to use that new version of babel-plugin-ember-modules-api-polyfill
    • to pass true or false to that new option based on the running version of ember-source
  • Update this PR to actually emit a ember module (current Ember apps actually have no ember module at runtime, we have to fix that)

Comment on lines 823 to 853
Object.defineProperty(context.exports, 'Ember', {
configurable: true,
writable: false,
get() {
return Ember;
},
});

return Ember;
},
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think we should redefine it. If we do, then you won't have a deprecation for every usage of Ember without importing (you'd only get on deprecation). This would make finding and fixing any global accessing quite difficult.

},
});

context.exports.Em = context.exports.Ember;
Copy link
Member

Choose a reason for hiding this comment

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

This one also needs to be Object.defineProperty(context.exports, 'Em' with a deprecation

@btecu
Copy link
Contributor

btecu commented Feb 1, 2021

Aren't there some important parts that have not been exposed via modules yet? How would that work with this?
For example Ember.onerror, is used for error trackers such as Bugsnag and Sentry.

@pzuraq
Copy link
Contributor

pzuraq commented Feb 1, 2021

@btecu as noted in the RFC, you will still be able to import Ember from 'ember';. This is just about accessing the object without importing it.

@NullVoxPopuli
Copy link
Contributor Author

@pzuraq
Copy link
Contributor

pzuraq commented Feb 10, 2021

@NullVoxPopuli can you rebase this PR? Now that #19390 has landed, everything should be much easier to deprecate since the global is now setup lazily (you can do it in @ember/-internals/bootstrap). Also we need to add a test that verifies the deprecation runs when we use the global value

… window.Ember

RFC: emberjs/rfcs#706

tl;dr: Recommend `import Ember from 'ember';` instead of using the window.Ember
global
@NullVoxPopuli NullVoxPopuli force-pushed the deprecate-window-global branch from bc14af2 to 5534e7f Compare February 13, 2021 20:26
@rwjblue
Copy link
Member

rwjblue commented Feb 16, 2021

https://github.com/ember-cli/babel-plugin-ember-modules-api-polyfill/releases/tag/v3.3.0 includes new useEmberModule option. Next up: update ember-cli-babel.

@simonihmig
Copy link
Contributor

This has been superseded by #19457, so can be closed, right?

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.

5 participants