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

fix: implement cacheKeyForTree that returns null #1424

Conversation

brendenpalmer
Copy link

fix: implement cacheKeyForTree that returns null

This fixes a bug between ember-cli-typescript, ember-engines dedupe, and ember-cli-babel. ember-cli-babel uses the existence of ember-cli-typescript as a dependency to determine whether to process *.ts files; however, ember-engines dedupe is stateful so it's possible for ember-cli-typescript to not be a dependency when ember-cli-babel is running.

For more info on ember-engines dedupe logic: https://github.com/ember-engines/ember-engines/blob/master/packages/ember-engines/lib/utils/deeply-non-duplicated-addon.js#L35

For more info on how ember-cli-babel determines whether to process *.ts: https://github.com/babel/ember-cli-babel/blob/master/lib/babel-options-util.js#L407

Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me! It's a bit of a shame we need to invalidate the caching here since it would otherwise be totally valid, but since we don't have any trees of runtime code the impact should be pretty minimal.

@brendenpalmer
Copy link
Author

@dfreeman I agree. One of the alternatives is to update ember-cli-babel to always transpile *.ts files, which is something I'm exploring in conjunction w/ this. If a fix in ember-cli-babel can land sooner than I expect, then I'll revert this PR (if it's merged by then), or close this out.

@jamescdavis
Copy link
Member

@brendenpalmer emberjs/ember-cli-babel#400 resolved this, right?

@brendenpalmer
Copy link
Author

@jamescdavis Yes, this has been resolved in ember-cli-babel itself, closing this PR out

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.

3 participants