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

@embroider/compat v1.9.0 breaks TypeScript components when staticComponents: true #1261

Closed
bwbuchanan opened this issue Oct 8, 2022 · 6 comments · Fixed by #1273
Closed

Comments

@bwbuchanan
Copy link

@ef4 @NullVoxPopuli

See my comment on #1236

#1236 changes the order of resolvableExtensions to put .ts at the end, instead of the beginning, of the array.

The stub template-only component .js file is then being resolved, instead of the actual component implementation in the .ts file, and the component breaks at runtime because its backing class is empty.

(Why are these template-only component stub files being generated in the first place, if an implementation file exists? Is this also a bug somewhere?)

@NullVoxPopuli
Copy link
Collaborator

👋 thanks for reporting!!

I don't remember too many details on this, but if you have the time, a PR that adds a test that fails with the scenario you describe would be immensely helpful <3

@ef4
Copy link
Contributor

ef4 commented Oct 8, 2022

(Why are these template-only component stub files being generated in the first place, if an implementation file exists? Is this also a bug somewhere?)

Yes, this is probably the real bug here. Previously, this comment was correct:

// we don't need to worry about all resolvable extensions in here (like .ts)
// because by the time we see the code it has already been preprocessed down to
// js.
const jsExtension = '.js';

But now it may not be.

@bwbuchanan
Copy link
Author

@ef4 it does seem that this comment is no longer accurate and the babel pass happens later.

I changed the code as below and embroider no longer generates template-only components in $TMPDIR/embroider when there is a TypeScript component file present.

Should we be checking for additional extensions here? .mjs, .mts?

But I think not.gjs and .gts, because they will not be used in conjunction with a .hbs file. Or should that be a warning/error case?

const jsExtensions = ['.ts', '.js'];

...

function crawl(dir: string) {
  let needed = new Set<string>();
  let seen = new Set<string>();
  if (pathExistsSync(dir)) {
    for (let file of walkSync(dir, { directories: false })) {
      if (file.endsWith(templateExtension)) {
        needed.add(file.slice(0, -1 * templateExtension.length));
      } else {
        const jsExtension = jsExtensions.find(ext => file.endsWith(ext));
        if (jsExtension) {
          seen.add(file.slice(0, -1 * jsExtension.length));
        }
      }
    }
  }
  return { needed, seen };
}

@evoactivity
Copy link
Contributor

I converted an app today to colocated components and ran in to this issue. Can confirm the fixes @bwbuchanan suggested have fixed my app.

nicolechung added a commit to nicolechung/ember-aria that referenced this issue Oct 26, 2022
Embroider optimized was failing. After looking into the issues, I found
this: embroider-build/embroider#1261

Changing new helper components I created in helpers.ts to
colocated components fixed this.
nicolechung added a commit to nicolechung/ember-aria that referenced this issue Oct 26, 2022
Embroider optimized was failing. After looking into the issues, I found
this: embroider-build/embroider#1261

Changing new helper components I created in helpers.ts to
colocated components fixed this.
@bwbuchanan
Copy link
Author

@ef4 Does my proposed fix look correct to you and should I submit a PR for this? Or are there other implications that need to be considered?

I changed the code as below and embroider no longer generates template-only components in $TMPDIR/embroider when there is a TypeScript component file present.

Should we be checking for additional extensions here? .mjs, .mts?

But I think not.gjs and .gts, because they will not be used in conjunction with a .hbs file. Or should that be a warning/error case?

const jsExtensions = ['.ts', '.js'];

...

function crawl(dir: string) {
  let needed = new Set<string>();
  let seen = new Set<string>();
  if (pathExistsSync(dir)) {
    for (let file of walkSync(dir, { directories: false })) {
      if (file.endsWith(templateExtension)) {
        needed.add(file.slice(0, -1 * templateExtension.length));
      } else {
        const jsExtension = jsExtensions.find(ext => file.endsWith(ext));
        if (jsExtension) {
          seen.add(file.slice(0, -1 * jsExtension.length));
        }
      }
    }
  }
  return { needed, seen };
}

@ef4
Copy link
Contributor

ef4 commented Oct 28, 2022

Yes, please file that PR, then we can see it get tested in CI.

bwbuchanan added a commit to bwbuchanan/embroider that referenced this issue Oct 29, 2022
steveszc added a commit to steveszc/catechesis that referenced this issue Oct 30, 2022
steveszc added a commit to steveszc/catechesis that referenced this issue Oct 30, 2022
steveszc added a commit to steveszc/catechesis that referenced this issue Oct 30, 2022
@ef4 ef4 closed this as completed in #1273 Oct 31, 2022
nicolechung added a commit to CrowdStrike/ember-aria-utilities that referenced this issue Nov 29, 2022
Before, {{aria-grid}} only worked for role=grid where the rows were all loaded at once. This now fixes that so it works for when the rows are loaded later (for example, when data needs to be fetched in order to populate the table).

added table examples
added async versions of all the examples where the rows are loaded loader
updated the MutationObserver to run without anything in the WeakMap. This Map looks like it was always empty by default and so the observer was not observing at all due to an early return. Not sure how this map works, happy to put it back in if it's needed.
updated row selectors to select tr tags not just role=row.
updated [role=cell] selectors to also check for td and th.
update [role=columnheader] to also check for th.
created new helper components as colocated components, due to: @embroider/compat v1.9.0 breaks TypeScript components when staticComponents: true embroider-build/embroider#1261
github-actions bot pushed a commit to CrowdStrike/ember-aria-utilities that referenced this issue Nov 29, 2022
# [2.3.0](v2.2.1...v2.3.0) (2022-11-29)

### Bug Fixes

* assertion to show correct amount of rows ([b848cda](b848cda))
* cell selectors for tbody only ([093030c](093030c))
* change new components to colocated components ([2ceda16](2ceda16))
* change port for docs, so docs and tests can be run at the same time ([17371dc](17371dc))
* formatting on sample data ([b2dcd7c](b2dcd7c))
* get table-tests in line with original grid tests ([aa9f894](aa9f894))
* indentation in table template ([69f5315](69f5315))
* make sure row operations runs at least once ([35d1457](35d1457))
* typo in firstCell selector ([10ac4b8](10ac4b8))
* update aria-grid selectors, MutationObserver logic ([d2a095b](d2a095b))
* update aria-grid test and add related table test ([42e383e](42e383e))
* update docs package json to use local package for ember-aria-utilities ([3239272](3239272))
* update table template in the test-app ([c116bf2](c116bf2))
* update tableSelectors ([f5724bc](f5724bc))
* updating tests ([b8838ee](b8838ee))

### Features

* Handle rows loading asynchronously, adding table selectors ([fe74ae8](fe74ae8)), closes [embroider-build/embroider#1261](embroider-build/embroider#1261)
* modify node selectors to work with table tags ([5f09b43](5f09b43))
* modify selectors to work for table tags ([04fcbc0](04fcbc0))
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