-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Internals Refactor #20092
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
Internals Refactor #20092
Conversation
afc1b91
to
a43b72c
Compare
isArray, | ||
} from './lib/mixins/array'; | ||
export { default as Comparable } from './lib/mixins/comparable'; | ||
export { NativeArray } from '@ember/array'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We re-export here since there are tests that want this to exist in @ember/-internals
. These should be revisited in the future.
export { default as ActionHandler } from './lib/mixins/action_handler'; | ||
export { default as _ProxyMixin, contentFor as _contentFor } from './lib/mixins/-proxy'; | ||
export { default as MutableEnumerable } from './lib/mixins/mutable_enumerable'; | ||
export { default as MutableEnumerable } from '@ember/enumerable/mutable'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We re-export here since there are tests that want this to exist in @ember/-internals
. These should be revisited in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagenet looking at these two comments—can you open an issue tracking it? Maybe an issue tracking both big and small work to be done for getting us to a point where we can publish individual packages (including for types), and just adding items to the list as we discover them until they're done? Or possibly a project doing the same?
401921a
to
b84d592
Compare
59e7bdb
to
e2b4008
Compare
Out of curiosity, is there anything holding this PR back? Seems like a lot of good stuff! |
e2b4008
to
dec0718
Compare
@sandstrom, just waiting on proper review! |
@wagenet Alright, great! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted a bunch of places where our @module
declarations in the docs seem to have been simply wrong, and AFAICT this is one part of why the actual generated module docs are substantially less useful than might have been hoped. Between your reorganization and adding those in, I think it might make a nice win for the docs while we're still on YUIDoc (though a colleague is working on doing the work to transition us to TSDoc-powered docs, almost certainly via TypeDoc). That said… it’s possible these are intentionally only showing the top-level module, which while deeply unhelpful may be important for some other reason at the moment?
cc. @jenweber @locks: do we want to have only (for example) @ember/routing
rather than @ember/routing/route
for the @module
declarations?
A related thought: we should consider whether we might want to introduce a public
/private
directory, so that you end up with this for input/output structure:
.
├── dist
│ ├── -private
│ │ ├── internal-only.d.ts
│ │ └── internal-only.js
│ └── public
│ ├── index.d.ts
│ ├── index.js
│ ├── other.d.ts
│ └── other.js
└── src
├── -private
│ └── internal-only.ts
└── public
├── index.ts
└── other.ts
Once this Typedoc issue is resolved, we can theoretically start leveraging package exports
maps to make it so consumers cannot use the -private
directories (if we so desire), with a very straightforward per-package config:
{
"exports": {
".": "./dist/public/index.js",
"./package.json": "./package.json",
"./*": "./dist/public/*.js"
}
}
packages/@ember/-internals/glimmer/lib/component-managers/mount.ts
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
Outdated
Show resolved
Hide resolved
import Controller, { inject as injectController } from '@ember/controller'; | ||
import { A as emberA, RSVP } from '@ember/-internals/runtime'; | ||
import { A as emberA } from '@ember/array'; | ||
import { RSVP } from '@ember/-internals/runtime'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably for a follow-on and/or just needs clarification: is this because we monkey-patch it or similar silliness, such that we cannot use the 'rsvp'
import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got extra stuff in @ember/-internals/runtime/lib/ext/rsvp.ts. Might still be fine to import from RSVP directly here though.
dec0718
to
afdd7ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more tweak, not a blocker!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it! …maybe starting by fixing CI? 😂
853f9f4
to
5feedc8
Compare
This consolidates a lot of stuff from @ember/-internals into its relevant packages. Not only is this better organizationally, but it puts us in a better place for correctly building type exports as it reduces the number of potential import locations for the same types.