-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add Typescript support #42
Changes from all commits
8b4162a
8e0dbd2
5c2e8e3
0c46c22
90b0672
598e2f3
7454c09
9afbea4
756eb0d
93f7be4
f2f6d1c
bfd5cd9
980ebad
eb2861b
583a2c5
c36da5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,5 @@ | |
|
||
module.exports = { | ||
singleQuote: true, | ||
printWidth: 100, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
{ | ||
"plugins": [ | ||
<% if (typescript) { %> "presets": [["@babel/preset-typescript"]], | ||
<% } %> "plugins": [ | ||
"@embroider/addon-dev/template-colocation-plugin", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. before I forget, I think we'll want to change the default settings:
This is what I use over here: https://github.com/NullVoxPopuli/ember-popperjs/blob/main/ember-popperjs/babel.config.cjs#L8 and cc @chriskrycho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NullVoxPopuli A few questions:
Re: the third bullet, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we need both -- preset-typescript for all TS things (and a general indicator to users, that we're using "TS stuff" for compilation -- and then the transform plugin to override declare fields.
catches typos with better errors at build time -- this is a personal preference, and it's a non issue if you just don't misspell things -- though maybe the error reporting has gotten better.
for optimizeConstEnums, I don't understand why anyone would want the previous behavior of an IIFE per-enum. For There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The preset sets up the transform for you, and accepts the same configuration
We aren't using
Sure, but Someone who knows they have a highly performance-sensitive situation and that it means they want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, nice! that would make the config even less verbose!
yeah, I was copy pasting from one of my own projects -- I had hoped that we could implicitly convert between all the babel config formats in the split of the comment, rather than the details of the comment 😅
fair enough 💯 |
||
["@babel/plugin-proposal-decorators", { "legacy": true }], | ||
"@babel/plugin-proposal-class-properties" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import babel from '@rollup/plugin-babel'; | ||
<% if (typescript) { %>import typescript from 'rollup-plugin-ts';<% } else { %>import babel from '@rollup/plugin-babel';<% } %> | ||
import copy from 'rollup-plugin-copy'; | ||
import { Addon } from '@embroider/addon-dev/rollup'; | ||
|
||
|
@@ -15,14 +15,20 @@ export default { | |
plugins: [ | ||
// These are the modules that users should be able to import from your | ||
// addon. Anything not listed here may get optimized away. | ||
addon.publicEntrypoints(['components/**/*.js', 'index.js']), | ||
simonihmig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
addon.publicEntrypoints(['components/**/*.js', 'index.js'<% if (typescript) {%>, 'template-registry.js'<% } %>]), | ||
|
||
// These are the modules that should get reexported into the traditional | ||
// "app" tree. Things in here should also be in publicEntrypoints above, but | ||
// not everything in publicEntrypoints necessarily needs to go here. | ||
addon.appReexports(['components/**/*.js']), | ||
|
||
// This babel config should *not* apply presets or compile away ES modules. | ||
<% if (typescript) { %> // compile TypeScript to latest JavaScript, including Babel transpilation | ||
typescript({ | ||
transpiler: 'babel', | ||
browserslist: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. browserslist false adds extra transpilation -- I was unable to determine why, but "last 1 firefox versions, last 2 chrome versions" has babel doing the least work There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I remember this discussion in your other PR. As I said in the PR description, I just added what worked for me, for now. When your PR is merged, I will update all of this! So let's not duplicate the same discussion here! 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record: as I already stated in the related embroider-build/embroider#1099, I think the config here is right ( Given that this does not break anything, it just produces code that is not as optimized as it could be, I would think to keep this as is now, and not expose workarounds to created addons that later need to get rolled back causing churn for maintainers, and rather rely on getting this fixed upstream eventually! |
||
transpileOnly: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should add declarationMap here as well -- I use this config: https://github.com/NullVoxPopuli/ember-resources/blob/main/ember-resources/config/rollup.config.js#L52 |
||
}), | ||
<% } else { %> // This babel config should *not* apply presets or compile away ES modules. | ||
// It exists only to provide development niceties for you, like automatic | ||
// template colocation. | ||
// | ||
|
@@ -31,7 +37,7 @@ export default { | |
babel({ | ||
babelHelpers: 'bundled', | ||
}), | ||
|
||
<% } %> | ||
// Follow the V2 Addon rules about dependencies. Your code can import from | ||
// `dependencies` and `peerDependencies` as well as standard Ember-provided | ||
// package names. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Easily allow apps, which are not yet using strict mode templates, to consume your Glint types, by importing this file. | ||
// Add all your components, helpers and modifiers to the template registry here, so apps don't have to do this. | ||
// See https://typed-ember.gitbook.io/glint/using-glint/ember/authoring-addons | ||
|
||
// import type MyComponent from './components/my-component'; | ||
|
||
// Remove this once entries have been added! 👇 | ||
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
export default interface Registry { | ||
// MyComponent: typeof MyComponent | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"extends": "@tsconfig/ember/tsconfig.json", | ||
simonihmig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"include": [ | ||
"src/**/*", | ||
"unpublished-development-types/**/*" | ||
], | ||
"glint": { | ||
"environment": "ember-loose" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Add any types here that you need for local development only. | ||
// These will *not* be published as part of your addon, so be careful that your published code does not rely on them! | ||
|
||
import '@glint/environment-ember-loose'; | ||
|
||
declare module '@glint/environment-ember-loose/registry' { | ||
// Remove this once entries have been added! 👇 | ||
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
export default interface Registry { | ||
// Add any registry entries from other addons here that your addon itself uses (in non-strict mode templates) | ||
// See https://typed-ember.gitbook.io/glint/using-glint/ember/using-addons | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this ember-cli issue is also relevant here: ember-cli/ember-cli#10046
Since both parsers have different options I think the
parserOptions
should also be applied conditionally?I was looking into fixing that in ember-cli but I'm not sure which options are needed exactly. I found this: https://typescript-eslint.io/docs/linting/typed-linting which sounds nice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed here. Eslint configs need to make heavy use of overrides, because we have a mix of node (cjs), node (esm), and browser (esm, js, ts).
Overrides are way easier to extend, change, target specific files, folders, etc.
It's also how my eslint-configs package can simultaneously support app, addon, v2 addon, js, ts, all from the same config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will wait for ember-cli/ember-cli#10054 to get merged before applying the same fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just applied the changes as in ember-cli/ember-cli#10054. Think that should be good.