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

Clarify how to set enableTypeScriptTransform in Ember apps, addons and engines #467

Merged
merged 3 commits into from
Aug 18, 2023
Merged

Clarify how to set enableTypeScriptTransform in Ember apps, addons and engines #467

merged 3 commits into from
Aug 18, 2023

Conversation

ijlee2
Copy link
Member

@ijlee2 ijlee2 commented Nov 24, 2022

Description

Currently, the README explains how to set enableTypeScriptTransform for Ember apps only.

It wasn't clear to me what to do for Ember addons and engines (to be precise, engines created as an addon). The setup for engines turned out to be tricky, because I wasn't supposed to embed the 'ember-cli-babel' key inside of an options hash (the pattern suggested by apps and addons).

I'd like to update the section Enabling TypeScript Transpilation so that:

  • A developer can copy-paste the code example.
  • The code examples closely match the latest Ember blueprint (v4.9.0-beta.0 at the time of writing). (Note, I continued to write return app.toTree();. That is, to limit the scope of this pull request, I didn't assume that the developer uses, or will use, Embroider.)
  • The surrounding texts have shorter sentences and are easier to understand. (The change may help those whose native language is not English.)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ijlee2
Copy link
Member Author

ijlee2 commented Dec 9, 2022

@chriskrycho When ember-cli-typescript is installed in a v1 addon, two scripts get added to package.json:

{
  "scripts": {
    "prepack": "ember ts:precompile",
    "postpack": "ember ts:clean"
  }
}

When we use enableTypeScriptTransform and uninstall ember-cli-typescript, is it okay to remove these two scripts (i.e. the addon and its types will be published without an issue)?

If so, I wonder if this is worth mentioning in ember-cli-babel's README. I can also see the perspective that it may fit better in a migration guide in the ember-cli-typescript repo.

@chriskrycho
Copy link
Contributor

Nope, nothing will generate types out of the box for you in that mode. For that reason, addons which want to publish types should generally do one of two things:

  • Classic addons should continue to use ember-cli-typescript.
  • Modern/Embroider addons should use the TS-ready rollup config for addons that @simonihmig and others built, which will publish types correctly.

Probably worth documenting that explicitly here (feel free to steal that prose and tweak/improve it!).

Copy link
Member

@bertdeblock bertdeblock left a comment

Choose a reason for hiding this comment

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

AFAICT, this looks good @ijlee2.

One thing I'm wondering though, I'm not sure if the ember-cli-build.js file for addons and engines should be modified as well. I think that enabling the transform from within the index.js file should be sufficient.

README.md Outdated

const EngineAddon = require('ember-engines/lib/engine-addon');

module.exports = EngineAddon.extend({
Copy link
Member

Choose a reason for hiding this comment

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

In v0.8.13, a buildEngine function was added to replace extend. It's also used in the docs, for example here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bertdeblock Thanks for the suggestion. I wasn't familiar with the new syntax and hope I updated the file correctly.

@bertdeblock bertdeblock changed the title Clarify how to set enableTypeScriptTransform in Ember apps, addons, and engines Clarify how to set enableTypeScriptTransform in Ember apps, addons and engines Jan 25, 2023
@bertdeblock bertdeblock self-assigned this Jan 25, 2023
@ijlee2 ijlee2 requested review from bertdeblock and NullVoxPopuli and removed request for bertdeblock and NullVoxPopuli February 24, 2023 11:41
@backspace
Copy link

Thanks for writing this up, it helped me when I was having build problems with an addon. I think it would be great to have merged so it’s easier to find 😀

@ijlee2
Copy link
Member Author

ijlee2 commented Jul 6, 2023

@bertdeblock Could we merge this pull request some time soon? (I forget where we had left off, if there had been additional feedback that I needed to address.)

@bertdeblock bertdeblock merged commit c343860 into emberjs:master Aug 18, 2023
@ijlee2 ijlee2 deleted the clarify-enableTypeScriptTransform branch August 28, 2023 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants