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

Use content-tag, which fixes *all the bugs* #187

Merged

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Aug 12, 2023

Learned some new cli-fu today:

❯ git diff --shortstat HEAD~1 -- . ':(exclude)yarn.lock' 
 33 files changed, 41 insertions(+), 2367 deletions(-)

the actual diff!

Breaking changes

all consumers that were importing from ember-template-imports will no longer be able to.
folks should typically want to use content-tag instead of ember-template-imports after this change.

Blockers


Extra bonus -- most of the code from the original implementation is gone now 🎉 yay deleting code!

(most of the additions are lockfile, btw)
((also most of the removals, but also 98% of the old code is gone, too))

This is a breaking change, however:

  • no more hbs support
  • all downstream dependencies importing from ember-template-imports will need to not import from ember-template-imports.

Related,

Not related,

  • I disabled the lint node/no-missing-require -- because it's buggy. and eslint-plugin-node hasn't been maintained for quite some time -- long term solution here is to switch to esilnt-plugin-n
  • I also disabled the lint node/no-unpublished-require -- because it doesn't understand how peer dependencies work.
resolved issues encountered while working on this PR

What I need help with,

  • why did I need to add @babel/plugin-transform-class-static-block? browsers support this (I got a babel error about needing to add this)
    • RESOLUTION: per use content-tag #182, I need to make the babel plugins peers -- it's intended that the plugins are needed due to the decorator transform our community is using.
  • looks like the test suite was maybe never passing against ember v5? -- it may take me a bit to figure out what's going on there
    UPDATE: I have things working on stackblitz -- so I don't yet know what's going on with C.I.:
    image

Supersedes #182

@NullVoxPopuli NullVoxPopuli force-pushed the use-the-real-implementation branch 5 times, most recently from 1499bb7 to 91869d7 Compare August 13, 2023 02:30
@NullVoxPopuli
Copy link
Collaborator Author

Failures exist over here: #190, so it has nothing to do with content-tag

@chriskrycho chriskrycho added enhancement New feature or request breaking labels Aug 16, 2023
@chriskrycho
Copy link
Collaborator

What does this mean?

all downstream dependencies importing from ember-template-imports will need to not import from ember-template-imports.

@NullVoxPopuli
Copy link
Collaborator Author

all downstream dependencies importing from ember-template-imports will need to not import from ember-template-imports.

What does this mean?

eslint-plugin-ember and ember-template-lint will need to remove their (peer)dependencies on ember-template-imports, because the util and GLIMMER_TEMPLATE constant have been removed.

prettier-plugin-ember-template-tag should be fine because it declares its own hard dependency on ember-template-imports.

There are paths forward for each of these, and @patricklx has been doing awesome work making legit lint tooling and I don't know what or if there is a story for combining efforts with content-tag, but the benefits, impact, and desire for getting the tools off of ember-template-imports is described here: #143

ember-addon-main.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@NullVoxPopuli
Copy link
Collaborator Author

There's a bit of bitrot, so I'm going to open some other PRs to try to make this easier.

  • ember-cli-babel v8 is needed
  • ember-cli-htmlbars needs upgraded
  • ember-qunit needs upgraded (to not use ember-cli-typescript)
    (this'll require some other upgrades as well, like ember-cli)

My hope is that I can avoid waiting for typed-ember/ember-cli-typescript#1569 as well, but timing might work out where it's no big deal.

@NullVoxPopuli
Copy link
Collaborator Author

Running in to:

Error: Could not find module `ember-compatibility-helpers` imported from `@glimmer/component/index`
                at missingModule (http://localhost:7357/assets/vendor.js:266:11)
                at findModule (http://localhost:7357/assets/vendor.js:277:7)
                at Module.findDeps (http://localhost:7357/assets/vendor.js:187:24)
                at findModule (http://localhost:7357/assets/vendor.js:281:11)
                at Module.findDeps (http://localhost:7357/assets/vendor.js:187:24)

I remember ember-compatibility-helpers having issues throughout the years.

The modern way do to what it wants to do is embroider/macros.
which.. would require a bunch more changes to other packages -- so hoping that's not needed

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Oct 14, 2023

update: by switching to pnpm,

  • patches arent needed
  • we dont need to release the other two packages
  • everything works

old comment:

We need to merge / release:

Gonna switch this project to pnpm in another PR tho, because yarn can't handle the state of our dependencies right now (locally, I have tests passing, but in yarn, I have the above ember-compatibility-helpers issue, which is typically due to incorrect duplicate dependencies in the dep graph)

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Oct 14, 2023

this PR now builds on top of: #203
so the diff here will be huge until #203 is merged

@NullVoxPopuli NullVoxPopuli force-pushed the use-the-real-implementation branch 2 times, most recently from 521b32a to 419cea4 Compare October 14, 2023 04:09
- ember-cli-babel 8.1+
  - required `@babel/core` (declared as peer)
- ember-cli-htmlbars 6.3.0+
  - allowed us to remove babel plugins from our dependencies
- content-tag 1.1.1+

and allows for:
- removal of all hbs-supporting code
  - removal of now-extraneous testing
- removal of custom processor
- removal of custom preprocessor plugin management

To get CI passing,
- ie11 support had to be removed in targets.js (latest ember does not
  support ie11)
- `@embroider/test-support` and `ember-try` had to be upgraded so they
  pull in supported dependencies today
@ef4 ef4 merged commit dc775e0 into ember-template-imports:master Oct 17, 2023
16 checks passed
@patricklx
Copy link
Contributor

It says it fixes all the bugs. But didn't close any GitHub issues :)

@NullVoxPopuli
Copy link
Collaborator Author

Oops. Thanks!

@NullVoxPopuli NullVoxPopuli deleted the use-the-real-implementation branch October 19, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants