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

Add Typescript support #42

Merged
merged 16 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
branches:
- main
schedule:
- cron: "0 3 * * 0" # every Sunday at 3am
- cron: '0 3 * * 0' # every Sunday at 3am

env:
CI: true
Expand All @@ -22,7 +22,7 @@ jobs:
run: pnpm lint

test_type_checking:
name: "Tests: Type Check"
name: 'Tests: Type Check'
timeout-minutes: 5
runs-on: ubuntu-latest

Expand All @@ -47,10 +47,10 @@ jobs:
- defaults with pnpm
- addon-location
- test-app-location
- typescript
steps:
- uses: actions/checkout@v3
- uses: ./.github/actions/pnpm
- run: pnpm add --global ember-cli yarn
- run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
working-directory: tests

1 change: 1 addition & 0 deletions .prettierrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@

module.exports = {
singleQuote: true,
printWidth: 100,
};
19 changes: 15 additions & 4 deletions files/__addonLocation__/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@

module.exports = {
root: true,
parser: '@babel/eslint-parser',
parser: '<%= typescript ? '@typescript-eslint/parser' : '@babel/eslint-parser' %>',
parserOptions: {
Copy link

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

ecmaVersion: 'latest',
ecmaVersion: 'latest',<% if (!typescript) { %>
sourceType: 'module',
ecmaFeatures: {
legacyDecorators: true,
},
babelOptions: {
root: __dirname,
},
},<% } %>
},
plugins: ['ember'],
extends: [
Expand All @@ -24,7 +24,18 @@ module.exports = {
},
rules: {},
overrides: [
// node files
<% if (typescript) { %> // ts files
{
files: ['**/*.ts'],
extends: [
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
],
rules: {
// Add any custom rules here
},
},
<% } %> // node files
{
files: [
'./.eslintrc.js',
Expand Down
3 changes: 2 additions & 1 deletion files/__addonLocation__/babel.config.json
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",
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

    [
      resolve('@babel/plugin-transform-typescript'),
      {
        allowDeclareFields: true,
        onlyRemoveTypeImports: true,
        // Default enums are IIFEs
        optimizeConstEnums: true,
      },
    ],

This is what I use over here: https://github.com/NullVoxPopuli/ember-popperjs/blob/main/ember-popperjs/babel.config.cjs#L8

and cc @chriskrycho

Copy link
Contributor

@dfreeman dfreeman Oct 17, 2022

Choose a reason for hiding this comment

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

@NullVoxPopuli A few questions:

  • Why plugin-transform-typescript instead of preset-typescript as Simon currently has?
  • Why resolve()?
  • Why default users to onlyRemoveTypeImports and optimizeConstEnums? I think both of those choices can be reasonable ones for an author to intentionally make, but I'm wary of diverging from default behavior in the blueprint unless there's a super compelling reason to do so.

Re: the third bullet, allowDeclareFields is a perfect example of something I do think is worth overriding for users by default in the blueprint: idiomatic Ember-in-TypeScript uses declare for decorated framework-managed fields, so folks can run into trouble pretty quickly if it's not set. (Also worth noting that Babel only defaults that one to false for backcompat reasons and is planning to default it to true in v8, if/when that day comes)

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Oct 17, 2022

Choose a reason for hiding this comment

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

Why plugin-transform-typescript instead of preset-typescript as Simon currently has?

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.

Why resolve()?

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.

Why default users to onlyRemoveTypeImports and optimizeConstEnums?

for optimizeConstEnums, I don't understand why anyone would want the previous behavior of an IIFE per-enum.

For onlyRemoveTypeImports, yeah, I think this is an artifact of a darker age in my babel config's history -- we don't need this one (and it implies confusing behavior, considering what the rollup stuff is doing)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need both

The preset sets up the transform for you, and accepts the same configuration

catches typos with better errors at build time

We aren't using resolve anywhere else in that file (and in fact we can't—it's .json). That feels like an unrelated change to this PR if you want to propose it.

for optimizeConstEnums, I don't understand why anyone would want the previous behavior of an IIFE per-enum

Sure, but const enum is somewhat of a power user feature with a lot of documented caveats and downsides (not to mention the "just use unions" crowd), so my argument here is that diverging from default behavior and introducing extra boilerplate config for maintainers isn't worth the tradeoff.

Someone who knows they have a highly performance-sensitive situation and that it means they want to use const enum is also likely to be able to determine that they want to change Babel's transpilation behavior as well. (Also, if you're in such a situation, actual consts + a union type will be more performant than optimizeConstEnums's output, since it skips an object field traversal)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Oh, nice! that would make the config even less verbose!

That feels like an unrelated change to this PR if you want to propose it.

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 😅

so my argument here is that diverging from default behavior and introducing extra boilerplate config for maintainers isn't worth the tradeoff.

fair enough 💯

["@babel/plugin-proposal-decorators", { "legacy": true }],
"@babel/plugin-proposal-class-properties"
Expand Down
53 changes: 45 additions & 8 deletions files/__addonLocation__/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"lint:hbs": "ember-template-lint . --no-error-on-unmatched-pattern",
"lint:js": "eslint . --cache",
"lint:hbs:fix": "ember-template-lint . --fix --no-error-on-unmatched-pattern",
"lint:js:fix": "eslint . --fix",
"lint:js:fix": "eslint . --fix",<% if (typescript) { %>
"lint:types": "glint",<% } %>
"start": "rollup --config --watch",
"test": "echo 'A v2 addon does not have tests, run tests in test-app'",
"prepack": "rollup --config"
Expand All @@ -29,12 +30,35 @@
},
"devDependencies": {
"@babel/core": "^7.17.0",
"@babel/eslint-parser": "^7.18.2",
<% if (typescript) { %>"@babel/preset-typescript": "^7.18.6"<% } else { %>"@babel/eslint-parser": "^7.18.2"<% } %>,
"@babel/plugin-proposal-class-properties": "^7.16.7",
"@babel/plugin-proposal-decorators": "^7.17.0",
"@babel/plugin-syntax-decorators": "^7.17.0",
"@embroider/addon-dev": "^1.0.0",
"@rollup/plugin-babel": "^5.3.0",
"@embroider/addon-dev": "^2.0.0",<% if (typescript) { %>
"@glint/core": "^0.9.7",
"@glint/environment-ember-loose": "^0.9.7",
"@tsconfig/ember": "^1.0.0",
"@types/ember": "^4.0.0",
"@types/ember__object": "^4.0.0",
"@types/ember__service": "^4.0.0",
"@types/ember__controller": "^4.0.0",
"@types/ember__string": "^3.16.0",
"@types/ember__template": "^4.0.0",
"@types/ember__polyfills": "^4.0.0",
"@types/ember__utils": "^4.0.0",
"@types/ember__runloop": "^4.0.0",
"@types/ember__debug": "^4.0.0",
"@types/ember__engine": "^4.0.0",
"@types/ember__application": "^4.0.0",
"@types/ember__test": "^4.0.0",
"@types/ember__array": "^4.0.0",
"@types/ember__error": "^4.0.0",
"@types/ember__component": "^4.0.0",
"@types/ember__routing": "^4.0.0",
"@types/ember__test-helpers": "^2.6.1",
"@typescript-eslint/eslint-plugin": "^5.30.5",
"@typescript-eslint/parser": "^5.30.5",<% } else { %>
"@rollup/plugin-babel": "^5.3.0",<% } %>
"concurrently": "^7.2.1",
"ember-template-lint": "^4.0.0",
"eslint": "^7.32.0",
Expand All @@ -44,7 +68,9 @@
"eslint-plugin-prettier": "^4.0.0",
"prettier": "^2.5.1",
"rollup": "^2.67.0",
"rollup-plugin-copy": "^3.4.0"
"rollup-plugin-copy": "^3.4.0"<% if (typescript) { %>,
"rollup-plugin-ts": "^3.0.2",
"typescript": "^4.7.4"<% } %>
},
"publishConfig": {
"registry": "https://registry.npmjs.org"
Expand All @@ -58,8 +84,19 @@
"main": "addon-main.js"
},
"exports": {
".": "./dist/index.js",
"./*": "./dist/*",
".": "./dist/index.js", <% if (typescript) { %>
"./*": {
"types": "./dist/*.d.ts",
"default": "./dist/*.js"
},<% } else { %>
"./*": "./dist/*.js",<% } %>
simonihmig marked this conversation as resolved.
Show resolved Hide resolved
"./addon-main.js": "./addon-main.js"
}
}<% if (typescript) { %>,
"typesVersions": {
"*": {
"*": [
"dist/*"
]
}
}<% } %>
}
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';

Expand All @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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! 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 (browserslist: false) as documented by rollup-plugin-ts, it's just that the latter seems to not behave correctly here. See the issue I filed here: wessberg/rollup-plugin-ts#189

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
//
Expand All @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions files/__addonLocation__/src/template-registry.ts
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
}
10 changes: 10 additions & 0 deletions files/__addonLocation__/tsconfig.json
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"
}
}
13 changes: 13 additions & 0 deletions files/__addonLocation__/unpublished-development-types/index.d.ts
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
}
}
88 changes: 62 additions & 26 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const sortPackageJson = require('sort-package-json');
const normalizeEntityName = require('ember-cli-normalize-entity-name');
const execa = require('execa');
const { merge } = require('lodash');
const { lt } = require('semver');

let date = new Date();

Expand All @@ -20,13 +21,42 @@ module.exports = {
description,

async afterInstall(options) {
let tasks = [this.createTestApp(options)];
let addonInfo = addonInfoFromOptions(options);
let testAppInfo = testAppInfoFromOptions(options);

/**
* Setup root package.json scripts based on the packager
*/
tasks.push(
(async () => {
let packageJson = path.join(options.target, 'package.json');
let json = await fs.readJSON(packageJson);

json.scripts = scripts(options);

await fs.writeFile(packageJson, JSON.stringify(json, null, 2));
})()
);

if (options.pnpm) {
tasks.push(pnpm.createWorkspacesFile(options.target, addonInfo, testAppInfo));
}

if (options.releaseIt) {
tasks.push(this.setupReleaseIt(options.target));
}

await Promise.all(tasks);
},

async createTestApp(options) {
const appBlueprint = this.lookupBlueprint('app');

if (!appBlueprint) {
throw new SilentError('Cannot find app blueprint for generating test-app!');
}

let addonInfo = addonInfoFromOptions(options);
let testAppInfo = testAppInfoFromOptions(options);
let testAppPath = path.join(options.target, testAppInfo.location);

Expand All @@ -44,39 +74,26 @@ module.exports = {

await appBlueprint.install(appOptions);

let tasks = [
await Promise.all([
this.updateTestAppPackageJson(path.join(testAppPath, 'package.json')),
this.overrideTestAppFiles(testAppPath, path.join(options.target, 'test-app-overrides')),
fs.unlink(path.join(testAppPath, '.travis.yml')),
];

/**
* Setup root package.json scripts based on the packager
*/
tasks.push(
(async () => {
let packageJson = path.join(options.target, 'package.json');
let json = await fs.readJSON(packageJson);

json.scripts = scripts(options);

await fs.writeFile(packageJson, JSON.stringify(json, null, 2));
})()
);
]);

if (options.pnpm) {
tasks.push(pnpm.createWorkspacesFile(options.target, addonInfo, testAppInfo));
}
if (options.typescript) {
const needsVersion = '4.9.0-beta.0';
const hasVersion = this.project.emberCLIVersion();

if (options.releaseIt) {
tasks.push(this.setupReleaseIt(options.target));
if (lt(hasVersion, needsVersion)) {
this.ui.writeWarnLine(
`Your version ${hasVersion} of Ember CLI does not support the --typescript flag yet. Please run \`ember install ember-cli-typescript\` in the ${testAppInfo.location} folder manually!`
);
}
}

await Promise.all(tasks);
},

async updateTestAppPackageJson(packageJsonPath) {
const pkg = require(packageJsonPath);
const pkg = await fs.readJSON(packageJsonPath);
const additions = require('./additional-test-app-package.json');

merge(pkg, additions);
Expand Down Expand Up @@ -105,11 +122,13 @@ module.exports = {
},

fileMapTokens(options) {
let { addonInfo, testAppInfo } = options.locals;
let { addonInfo, testAppInfo, ext, typescript } = options.locals;

return {
__addonLocation__: () => addonInfo.location,
__testAppLocation__: () => testAppInfo.location,
__ext__: () => ext,
__rollupExt__: () => (typescript ? 'mjs' : 'js'),
};
},

Expand All @@ -136,6 +155,7 @@ module.exports = {
options.testAppLocation && `"--test-app-location=${options.testAppLocation}"`,
options.testAppName && `"--test-app-name=${options.testAppName}"`,
options.releaseIt && `"--release-it"`,
options.typescript && `"--typescript"`,
]
.filter(Boolean)
.join(',\n ') +
Expand All @@ -158,13 +178,29 @@ module.exports = {
pnpm: options.pnpm,
npm: options.npm,
welcome: options.welcome,
typescript: options.typescript,
ext: options.typescript ? 'ts' : 'js',
blueprint: 'addon',
blueprintOptions,
ciProvider: options.ciProvider,
pathFromAddonToRoot,
};
},

files(options) {
let files = this._super.files.apply(this, arguments);

if (options.typescript) {
return files;
} else {
let ignoredFiles = ['__addonLocation__/tsconfig.json'];

return files.filter(
(filename) => !filename.match(/.*\.ts$/) && !ignoredFiles.includes(filename)
);
}
},

normalizeEntityName(entityName) {
entityName = normalizeEntityName(entityName);

Expand Down
Loading