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

Template tag codemod feedback #2077

Closed
kategengler opened this issue Aug 23, 2024 · 7 comments · Fixed by #2133
Closed

Template tag codemod feedback #2077

kategengler opened this issue Aug 23, 2024 · 7 comments · Fixed by #2133

Comments

@kategengler
Copy link

kategengler commented Aug 23, 2024

  1. Comments don't stay with their lines:
// eslint-disable-next-line my-rule
const foo = somethingIShouldNotDo();

becomes

// eslint-disable-next-line my-rule
import featureFlag_ from 'ember-feature-flags/helpers/feature-flag';
import ...
//...
cosnt foo = somethingIshouldntDo();

This isn't a problem so much for lints, because the errors make them findable, but for domain comments this is hard to find when the template tag codemod is run in bulk.

  1. Imports with _ appended are ugly. I understand this is to not conflict, but I'd prefer the option to not have this and then deal with conflicts on my own.
  2. I'd prefer imports of components to start with a capital letter -- maybe this could be an option?
  3. Document that the prettier setup for g(t|j)s is expected to be done before the codemod is run
  4. Many import paths come out including /ambiguous -- I assume this means the codemod couldn't find the import. This at the least should be documented to explain. The sheer number of these mean that I cannot run the codemod once but need to do it component-by-component over time.
@void-mAlex
Copy link
Collaborator

to try and manage expectations:

  1. will investigate how to better make those inserts for the imports; there may be tradeoffs to have multiple lines bringing in imports from the same source eg @ember/modifiers and they would not be grouped together
  2. working on a better default and option for customizability. plan is to make from: Shared::ConfirmButton become Shared_ConfirmButton keeping path to component
  3. see above
  4. prettier is a code mod concern and should be set up as part of enabling g(j|t)s in a project; running prettier . after the codemod would then fix up formatting
  5. fix is incoming

@amk221
Copy link

amk221 commented Sep 23, 2024

Thanks for the codemod. I'm experiencing a dead-end when I run it

Build Error (TemplateTagCodemodPlugin)

unknown file: Cannot read properties of undefined (reading 'addComment')

@amk221
Copy link

amk221 commented Oct 22, 2024

Is there any chance you can remove the /index from the import paths

e.g. prefer:

import MyComponent from 'app/components/my-component';

rather than

import MyComponent from 'app/components/my-component/index';

@void-mAlex
Copy link
Collaborator

@amk221 I assume you're using pods to have gotten that import path as normally you'd not see index there
if that's not the case I would love to see an example of what produces that output, but in all likelyhood you're getting that because it's the actual path (which would make it forward compatible with other tools) not something remapped;
the aim is to bring things towards standard out of the box compat with rest of the ecosystem

@amk221
Copy link

amk221 commented Oct 22, 2024

We are using pods, but afaik pods is not to do with components. But yes, inside my-component directory would be:

index.js
index.hbs
index.scss

This used to be the guidance from ember, I think it was called co-location.

If that is not the done-thing any more, I'll have to find a new place to put the co-located css

Edit But regadless, I understand it's not a concern of your codemod, so I'm ok with manually removing /index from the import paths

@NullVoxPopuli
Copy link
Collaborator

@amk221 that is still acceptable for co-located components.

@void-mAlex
Copy link
Collaborator

@amk221 I would not recommend removing /index without also moving the files a level higher
as @NullVoxPopuli said that will work just as well having a .scss at the same level as your new .gjs file

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 a pull request may close this issue.

4 participants