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

Adopt builtin components #1

Merged
merged 13 commits into from Nov 10, 2021
Merged

Adopt builtin components #1

merged 13 commits into from Nov 10, 2021

Conversation

jamemackson
Copy link
Contributor

@jamemackson jamemackson commented Feb 27, 2021

description

in support of RFC 0671, this PR aims to move the checkbox, textarea, text-field, and link-to components into this addon.

@chancancode
Copy link
Member

We don't need to re-export the components in the app folder. We only want the components to be importable from the addon, but we don't want them to be automatically resolvable under those names, otherwise they will shadow the ones from ember.

@jamemackson
Copy link
Contributor Author

jamemackson commented Mar 1, 2021

@chancancode I have removed the re-exports now, thanks for the quick feedback on that.

I did want to confirm that it's the link-to component that's being referred to by @ember/routing/link-component in the RFC.

I have run into a few imports that I believe are private or otherwise not exported from their source package, such as these from the link-to component. Do you have any recommendation on how I might proceed with handling those? I could always PR the packages to export them if needed but I'm guessing that's not desirable or where possible we could look at porting the related thing into this addon as well but that's likely not possible for some of these...

// private in link-to
import RouterState from '@ember/-internals/routing/lib/system/router_state';
import { isSimpleClick } from '@ember/-internals/views';
import { flaggedInstrument } from '@ember/instrumentation';
import { HAS_BLOCK } from '../component-managers/curly';
// private in text-field
import { hasDOM } from '@ember/-internals/browser-environment';

@mixonic
Copy link
Sponsor Member

mixonic commented Nov 9, 2021

I've landed a CI suite for this addon which tests against Ember 3.28.x stable, and against a build of the branch emberjs/ember.js#19806.

@mixonic mixonic force-pushed the rfc0671-move-builtin-components branch from a0171d9 to 15c0745 Compare November 9, 2021 23:11
@mixonic mixonic force-pushed the rfc0671-move-builtin-components branch from da7d7d4 to d9d0224 Compare November 10, 2021 00:36
@mixonic
Copy link
Sponsor Member

mixonic commented Nov 10, 2021

Ok. There is a CI build, and it is green. What have we done:

  • This branch contains a fork of Ember components from around... 3.25?
  • Partially the code is in TypeScript, mostly it is, but the types are failing all over.
  • The linter for this repo is still the Ember addon default despite the code not being traditional addon code, so the linter has simply been disabled.
  • An earlier commit in this PR contained all the tests for these interfaces from Ember. I've deleted them, as they were reliant on a test harness we don't want to port out of Ember.
  • The components in this repo may still perform deprecated behaviors we don't actually want, I'm unsure.
  • There are new and extremely minimal tests covering basic sanity.
  • The CI suite runs on Ember 3.28 and on a build of [CLEANUP beta] Drop export of built-ins, remove legacy components ember.js#19806, so I believe that means the addon is basically working as intended.

I'm not sure we're done with this work, but I'm inclined to merge this PR and encourage others to help close the gap working against main.

@chancancode @jamemackson @rwjblue what do you think?

@mixonic mixonic changed the title WIP: move builtin components Adopt builtin components Nov 10, 2021
@mixonic mixonic merged commit ea75628 into main Nov 10, 2021
@mixonic mixonic deleted the rfc0671-move-builtin-components branch November 10, 2021 18:45
@mixonic
Copy link
Sponsor Member

mixonic commented Nov 10, 2021

Let's do it. Thank you @jamemackson for getting this going!

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.

None yet

3 participants