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

Component name lookup failing #339

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tylerturdenpants
Copy link

@tylerturdenpants tylerturdenpants commented Nov 29, 2019

this 99% fixes #335, without "unique paths". This is to provide a stop gap until the "js for every component" solution is provided

@tylerturdenpants
Copy link
Author

@knownasilya this may be a reasonable work around.

@webark
Copy link
Owner

webark commented Dec 2, 2019

thanks for putting this together. Just one requested organizational change.

@webark
Copy link
Owner

webark commented Dec 2, 2019

https://github.com/ebryn/ember-component-css/blob/aa2ce7a5aeea65fefef7d5096dda4a051e8ce3a6/addon/utils/add-component-style-namespace.js#L2-L21

This is how i’m handling the this in the “new world order” btw

Edit:: well, i mean the general intent would be to import the class name and apply it directly, but this was intended as the “catch all” to ease adoption. (except this doesn’t work for addon components that are not re exported into the “app” space)

@tylerturdenpants
Copy link
Author

Sorry, i'm not sure if I haven't had enough coffee or if I'm having tunnel vision...but what is the organizational change you wont me to make? 😪

@webark
Copy link
Owner

webark commented Dec 2, 2019

@tylerturdenpants something like..

function identifierFromLayoutModuleName(modulePath = '') {
  const terminator = 'components/';
  const pathSegementToRemove = /.+\/components\//;

  return modulePath.replace(/\.\w+$/, '')
    .substr(modulePath.lastIndexOf(terminator) + terminator.length)
    .replace(pathSegementToRemove, '')
    .replace(/\/template/, '');
}

function identifierFromDebugContainerKey(debugContainerKey = '') {
  return debugContainerKey.replace('component:', '');
}

Component.reopen({
  _componentIdentifier: computed({
    get() {
      return identifierFromLayoutModuleName(this.get('layout.referrer.moduleName') || identifierFromDebugContainerKey(this._debugContainerKey);
    }
  }),

  ...
  
  styleNamespace: computed({
    get() {
      return podNames[this.get('_componentIdentifier')] || '';
    }
  }),

  ...

});

@tylerturdenpants
Copy link
Author

Got it!

@tylerturdenpants
Copy link
Author

I got a little confused by the other things that you were talking about. But here you go!

@tylerturdenpants
Copy link
Author

@webark if you could clear the CI cache that would be awesome. The core-js authors had a package foobar - zloirock/core-js#710

@webark
Copy link
Owner

webark commented Dec 3, 2019

Done (I think)

@tylerturdenpants
Copy link
Author

@webark so Travis is still acting funky. The builds will pass but all caches need to be cleared and the CI restarted. I face similar issues with my own projects. I’d fix this but I can’t do anything and Travis to fix it. Let me know if you need anything else. I know there will be a lot of happy people once this lands.

@webark
Copy link
Owner

webark commented Dec 3, 2019

@tylerturdenpants looks like it’s going through this time.

@webark
Copy link
Owner

webark commented Dec 3, 2019

I don’t love the skipped test that was introduced. I’ll look into this tomorrow, and then merge it either way.

@webark
Copy link
Owner

webark commented Dec 3, 2019

thanks so much for this!!!

@tylerturdenpants
Copy link
Author

tylerturdenpants commented Dec 3, 2019

The skipped test was to start a dialogue on wether to support paths that are non-standard or fall under MU. This approach only works if non-standard paths are not used - I think this usage is likely so low it’s not worth carrying through to a 1.0 release (my opinion 😝 ). I had to make similar concessions while working on the angle-bracket codemod

@tylerturdenpants
Copy link
Author

tylerturdenpants commented Dec 4, 2019 via email

@webark
Copy link
Owner

webark commented Dec 6, 2019

sorry. will today.

@webark
Copy link
Owner

webark commented Dec 9, 2019

If i am unable today, i’ll just release this as a beta until i have a chance to look into the unsupported paths.

@tylerturdenpants
Copy link
Author

No worries. A beta will great. If you need anything more from me, let me know. I’m curious about the use case for unsupported paths (and supporting them). If you can point me to any documentation, blog, RFC, etc about unsupported paths so that I can get more insight about them I’d really appreciate it.

@webark
Copy link
Owner

webark commented Dec 9, 2019

#238

and the corresponding issue have examples.

@tylerturdenpants
Copy link
Author

thanks! Now that I have some concrete background on this usage, I think I can look into finding a fix alongside you.

@NullVoxPopuli
Copy link

does this also fix component-co-location?

@tylerturdenpants
Copy link
Author

@NullVoxPopuli as you can see I borrowed logic from telemetry helpers to inspect the module and it’s path. I think this might be the only way to identify the component name by applying some heuristic to the module name. Do you know how we could do that with component co-location? I imagine it should be straight forward.

@tylerturdenpants
Copy link
Author

Plus I haven’t had time to dig into this. Holiday Madness!!!

@webark
Copy link
Owner

webark commented Jul 11, 2020

@tylerturdenpants are you still using this addon? or have you moved off of it due to this issue? Just finally coming back around to these.

@tylerturdenpants
Copy link
Author

@webark yes very much so. We are blocked on upgrading past 3.12. The team at work has double in size and this is the de-facto way that we style and probably why we haven’t moved on to another addon. I willing to help where you need me.

@webark
Copy link
Owner

webark commented Jul 11, 2020

ok cool. are you happening to use them in addons? or just apps?

@tylerturdenpants
Copy link
Author

Just apps.

@tylerturdenpants
Copy link
Author

Actually we have it has a dependency in a private addon

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.

Doesn't work on components without a JS class in 3.13
3 participants