Skip to content

Application class: convert docs to ES6 syntax#20113

Merged
chancancode merged 1 commit intoemberjs:masterfrom
tniezurawski:patch-1
Jun 21, 2022
Merged

Application class: convert docs to ES6 syntax#20113
chancancode merged 1 commit intoemberjs:masterfrom
tniezurawski:patch-1

Conversation

@tniezurawski
Copy link
Contributor

@tniezurawski tniezurawski commented Jun 11, 2022

I think we could update the docs on the Application class to ES6 syntax. Although, I believe the docs are not wrong and you still can .extend() but having ES6 syntax would make it more modern and be in line with the current blueprint: https://github.com/ember-cli/ember-cli/blob/v4.4.0/blueprints/app/files/app/app.js

@tniezurawski
Copy link
Contributor Author

Could someone help me in understanding why the CI / Browserstack Tests (Safari, Edge) test is failing?

@chancancode chancancode changed the base branch from release to master June 14, 2022 00:35
@chancancode
Copy link
Member

I think that test suite is (unfortunately) know to be a bit flaky so it's probably not something your PR caused. Can you rebase your changes against the master branch? You previously targeted the release branch, but that branch get recreated every time a new release comes out (which just happened today). In general it's better to target the master branch for that reason. I tried changing the base in the UI, but that doesn't seem to be sufficient in this case.

@tniezurawski
Copy link
Contributor Author

Thanks @chancancode. I rebased the change against master 👍 I think I followed this button before:

image
🔗 https://api.emberjs.com/ember/release/classes/Application

Strangely enough, when I click it now I see:

image

Is this something that should be addressed as well? I see the link is pointing to release and js file:

https://github.com/emberjs/ember.js/edit/**release**/packages/@ember/application/lib/**application.js**

maybe it should be changed this way?

- https://github.com/emberjs/ember.js/edit/release/packages/@ember/application/lib/application.js
+ https://github.com/emberjs/ember.js/edit/master/packages/@ember/application/lib/application.ts

WDYT?

@tniezurawski
Copy link
Contributor Author

@chancancode Any chance to get this merged?

@chancancode
Copy link
Member

Thank you!

Is this something that should be addressed as well? I see the link is pointing to release and js file:

Ah, thank you for pointing that out. That's a trickier question and I'll defer to @emberjs/learning-team-managers. There probably isn't an easy answer. The link is technically pointing you to the correct place, and in a sense that's the only correct place to point at, in that the content you saw rendered on that page is based on the source code of the release branch (the same page could have been moved/rewritten/deleted on the master branch, to be deployed when the next version is released, for example). It's just that as far as the development workflow of this repository, we typically develop against the master branch and cherry-pick changes down to beta/release as needed/appropriate.

@chancancode chancancode merged commit 64de34f into emberjs:master Jun 21, 2022
}
```

Because `Application` ultimately inherits from `Ember.Namespace`, any classes
Copy link
Member

Choose a reason for hiding this comment

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

@tniezurawski if you are interested in doing more edits for this class, I feel like most of this Ember.Namespace and "Application as a container that holds the other classes in your application" stuff should be deleted.

It was referring to to globals resolver which is deprecated/removed at this point. I thought we had deprecated Ember.Namespace as well but apparently we didn't. In any case, it doesn't really do anything useful at this point so we don't need to mention it here.

Copy link
Member

Choose a reason for hiding this comment

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

The "Event Delegation" stuff probably should go as well though it requires slightly more thoughts on what to do with the content (straight up delete it, or moving it somewhere else, etc).

Most idiomatic Ember apps uses {{on}} which doesn't go through event delegation at all and is equivalent to addEventListener directly on the element. The event delegation stuff is still used by classic components (@ember/components) so it's not completely gone, per se, but definitely doesn't need to be mentioned so front and center here, and setting it largely has no effect at all for a lot of modern apps.

Maybe we can keep it as a @property level documentation, or maybe it's more appropriate to talk about this in details in the docs for @ember/component.

cc @emberjs/learning-team-managers

@tniezurawski tniezurawski deleted the patch-1 branch July 15, 2022 20:19
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.

2 participants