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-only-glimmer-components: Add support for pods and mixed layouts #97

Merged
merged 4 commits into from
May 9, 2019

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented May 9, 2019

Resolves one part of #7

/cc @chancancode @rwjblue

Turbo87 added 4 commits May 9, 2019 10:30
…eration

instead, we will iterate over the templates that we found, generate the path to the corresponding JS file and check if it exists directly without needing the glob results
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

There is a decent amount of duplication here between the "pods without prefix" and "pods with prefix", can we remove it? Also, the implementation doesn't currently support all pods formats (e.g. app/some-component-name/template.hbs).

At first, I thought that removing the two branches in favor of something like this would solve both scenarios, avoid duplication, and support the missing pods scenario (app/some-component-name/template.hbs), but this has the issue of adding component.js files for route templates which is annoying.

    componentsRoot = path.join(root, `app/${podsFolder}/components`);
    templateCandidates = yield p(glob)('**/template.hbs', { cwd: 'app' });

    templateCandidates.forEach(template => {
      let templatePath = path.join('app', template);

      let jsPath = path.join('app', template.replace(/template\.hbs$/, 'component.js'));
      if (fs.existsSync(path.join(root, jsPath))) return;

      let tsPath = path.join('app', template.replace(/template\.hbs$/, 'component.ts'));
      if (fs.existsSync(path.join(root, tsPath))) return;

      templates.push(templatePath);
      components.push(jsPath); // Always offer to create JS
    });

🤔

@Turbo87
Copy link
Member Author

Turbo87 commented May 9, 2019

There is a decent amount of duplication here between the "pods without prefix" and "pods with prefix", can we remove it?

we could, but I'm not sure the code will be that much easier to read afterwards. since it's only used twice I felt that it wasn't worth extracting a function for it

doesn't currently support all pods formats (e.g. app/some-component-name/template.hbs).

I am not aware of that format. I was under the impression that for pods the templates always have to be in the components folder.

Even without support for this particular format this seems like an incremental improvement and we can add support for that other format later?

@rwjblue
Copy link
Member

rwjblue commented May 9, 2019

Yep, agreed!

@rwjblue rwjblue merged commit 226c956 into emberjs:master May 9, 2019
@rwjblue rwjblue added the enhancement New feature or request label May 9, 2019
@rwjblue rwjblue mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants