Skip to content

V2 Addon Conversion#1471

Merged
NullVoxPopuli merged 10 commits intomasterfrom
the-v2-addon-conversion
Aug 20, 2024
Merged

V2 Addon Conversion#1471
NullVoxPopuli merged 10 commits intomasterfrom
the-v2-addon-conversion

Conversation

@NullVoxPopuli
Copy link
Copy Markdown
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jul 29, 2024

PRs to extract:


Steps:

  • cd addon
  • Upgrade local ember-cli
  • npx ember-cli@latest init --blueprint @embroider/addon-blueprint --typescript --addon-only
  • work through diffs
  • add file extensions to all imports (required by the default rollup config)
  • pnpm lint:fix
  • manually address / disable remaining lint failures

// @ts-ignore
import { precompileTemplate } from '@ember/template-compilation';

const OUTLET_TEMPLATE = precompileTemplate(`{{outlet}}`, { strictMode: false });
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to import from ember-cli-htmlbars, so I used @ember/template-compilation for precompileTemplate

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This could also be pulled out to a separate PR, if folks want

// here; also see https://github.com/emberjs/data/issues/4071 for context
let setupContainer = require('ember-data/setup-container')['default'];
setupContainer(owner);
if (macroCondition(dependencySatisfies('ember-data', '>= 2.3'))) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This change is also meaningful -- we don't want to use requirejs -- we're trying to get rid of it.

Insetad, our abstraction over runtime module loader is provided by embroider-macros.

Additionally, I saw that in this file that setup-container is deprecated without replacement for removal in v6.

So I added another if condition around this code so we can hopefully be a little fault tolerant when ember-data v6 comes around.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This could also be pulled out to a separate PR, if folks want

@NullVoxPopuli NullVoxPopuli force-pushed the the-v2-addon-conversion branch from 6d7e763 to 3c062f6 Compare August 20, 2024 14:46
@NullVoxPopuli NullVoxPopuli force-pushed the the-v2-addon-conversion branch from 3c062f6 to d62c5cf Compare August 20, 2024 14:51
It doesn't make sense to run tests for type-tests with ember-try, since they're in a different workspace and not using the app

Remove Glint
Need multi-lockfile thanks to TS
Copy link
Copy Markdown
Member

@wagenet wagenet left a comment

Choose a reason for hiding this comment

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

This seems likely to be fine. Hard to review!

@NullVoxPopuli NullVoxPopuli merged commit 381c22c into master Aug 20, 2024
@NullVoxPopuli NullVoxPopuli deleted the the-v2-addon-conversion branch August 20, 2024 15:33
@github-actions github-actions bot mentioned this pull request Aug 20, 2024
@NullVoxPopuli NullVoxPopuli mentioned this pull request Aug 20, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants