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

data-test selectors are not stripped from colocated component templates #427

Closed
kpfefferle opened this issue Oct 29, 2019 · 20 comments · Fixed by #521
Closed

data-test selectors are not stripped from colocated component templates #427

kpfefferle opened this issue Oct 29, 2019 · 20 comments · Fixed by #521
Labels

Comments

@kpfefferle
Copy link

I noticed that not all data-test-* selectors are being stripped from my production build. It seems that the commonality among those that remain is that the selectors that remain are all from component templates that are using the new colocated component file structure (i.e. having the component.hbs file inside the app/component directory alongside the corresponding component.js file.

@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 29, 2019

I honestly have no idea what to do about this.

@chancancode @rwjblue any clues?

@chancancode
Copy link
Contributor

I assume this is implanted as an AST transform? Sounds like that is not being run for some reason? What version of ember-cli-htmlbars?

@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 29, 2019

we're still on v3 of htmlbars since we haven't dropped Node 6 support yet 🤔

@chancancode
Copy link
Contributor

I would think that it’s the app’s ember-cli-htmlbars version that matters in this case, but I’m not totally sure. @rwjblue?

@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 30, 2019

I would assume that co-location wouldn't work at all then 🤔

@rwjblue
Copy link
Contributor

rwjblue commented Nov 1, 2019

@kpfefferle - What ember-cli-htmlbars version is your application using? There were a number of bugs in the implementation around AST plugins not running properly, but as far as I can tell all reported issues have been addressed and things are working properly with the latest release (v4.0.8 atm). If you are on the latest version, I'm happy to dig into a reproduction.

@kpfefferle
Copy link
Author

@rwjblue Our application is using ember-cli-htmlbars@^4.0.8. I'll see if I can reproduce this with a new Octane app.

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 15, 2019

@kpfefferle any news on this?

@kpfefferle
Copy link
Author

@Turbo87 I can confirm I'm still seeing this issue when building our app for production. I haven't had the bandwidth to try and reproduce in a new sharable Octane app yet.

@rwjblue
Copy link
Contributor

rwjblue commented Dec 20, 2019

Please let us know, I'm very interested in figuring out what is going on here. There were absolutely bugs in the early 4.x series that caused AST plugins to not be ran properly, but as far as I can tell (from issues reported over there) they have all been addressed.

@void-mAlex
Copy link

ran into this issue. anything I can to to get this moving along or is someone already digging into it?

@Turbo87
Copy link
Collaborator

Turbo87 commented Feb 26, 2020

a reproduction repository with instructions would be good :)

@void-mAlex
Copy link

@raido
Copy link
Contributor

raido commented Feb 27, 2020

I am experiencing this as well. All packages are on latest possible versions, except maybe some transitive dependencies.

@jrjohnson
Copy link
Contributor

jrjohnson commented Apr 19, 2020

This is still an issue with a brand new ember app today. I put some debuggers in @void-mAlex's reproduction (and a nearly identical one with a brand new app). All the options seem to be working, but https://github.com/simplabs/ember-test-selectors/blob/master/strip-test-selectors.js#L16 is never called.

--edit--
Dug a bit further and it seems the walker does not visit any nodes inside of the component. It only sees the ElementNode that is the component, but it doesn't report any children and those children are never visited.

@Turbo87 Turbo87 added the bug label Apr 19, 2020
@Turbo87
Copy link
Collaborator

Turbo87 commented Apr 19, 2020

I can confirm that StripTestSelectorsTransform.prototype.transform is not ever run for any of the colocated templates. I can reproduce it in https://github.com/rust-lang/crates.io too 😢

@rwjblue any ideas?

@patocallaghan
Copy link

@Turbo87 Just wondering if there has been any progress on figuring this one out?

@Turbo87
Copy link
Collaborator

Turbo87 commented May 6, 2020

not from my side. I think it's a bug in the build pipeline somewhere, but not related to the implementation here.

@rwjblue
Copy link
Contributor

rwjblue commented May 6, 2020

ZOMG, I spent a while debugging this 😭, and have a fix. Basically, what is happen (in order) is:

  1. ember-cli-htmlbars's included hook runs, and does:
    1a. gather all AST plugins registered as htmlbars-ast-plugin in the registry
    1b. add babel-plugin-htmlbars-inline-precompile to the babel configuration
  2. ember-test-selectors's included hook runs, and registers a new htmlbars-ast-plugin

Specifically, the babel config is already locked down before ember-test-selectors. I do not think this issue is related to colocation at all (I doubt this ever ran against inline compiled templates), but the fact that colocation happens to desugar into using inline templates (e.g. hbs) makes it much more common to care.

The simplest fix, is to ensure that ember-test-selectors runs before ember-cli-htmlbars, and I've confirmed that works when manually tweaked into void-mAlex/test-selector-repro. See #521.

@rwjblue
Copy link
Contributor

rwjblue commented May 6, 2020

FWIW, I'm working on adding some additional logging in ember-cli-htmlbars to make debugging things like this a bit easier: ember-cli/ember-cli-htmlbars#547.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants