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.

Sign up for GitHub

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

Jump to bottom

Add support for @tsconfig/ember 3.0.0 #614

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Add support for @tsconfig/ember 3.0.0 #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82986dc
Fix ember try scenarios failing
Jul 26, 2023
4b3ff92
Add @tsconfig/ember to test-app
Jul 26, 2023
8498471
Add support for @tsconfig/ember 3.0.0
Jul 26, 2023
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
18 changes: 15 additions & 3 deletions addon/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,21 @@
"license": "MIT",
"author": "Santiago Ferreira",
"exports": {
".": "./dist/index.js",
"./*": "./dist/*",
"./addon-main.js": "./addon-main.js"
".": {
"import": "./dist/index.js",
"require": "./dist/index.js",
"types": "./types.d.ts"
},
"./*": {
"import": "./dist/*",
"require": "./dist/*",
"types": "./types.d.ts"
},
"./addon-main.js": {
"import": "./addon-main.js",
"require": "./addon-main.js",
"types": "./types.d.ts"
}
},
"types": "types.d.ts",
"files": [
Expand Down
8 changes: 7 additions & 1 deletion pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions test-app/config/ember-try.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ module.exports = async function () {
name: 'ember-release',
npm: {
devDependencies: {
'@ember/string': '^3.1.1',
'ember-source': await getChannelURL('release'),
},
},
Expand All @@ -36,10 +37,29 @@ module.exports = async function () {
}),
},
},
{
name: 'ember-release-typescript-5',
npm: {
devDependencies: {
'@ember/string': '^3.1.1',
'@tsconfig/ember': '^3.0.0',
'ember-cli-typescript': '^5.2.1',
'ember-resolver': '^10.1.1',
'ember-source': await getChannelURL('release'),
typescript: '^5.1.6',
},
},
env: {
EMBER_OPTIONAL_FEATURES: JSON.stringify({
'jquery-integration': false,
}),
},
},
{
name: 'ember-beta',
npm: {
devDependencies: {
'@ember/string': '^3.1.1',
'ember-source': await getChannelURL('beta'),
},
},
Expand Down
1 change: 1 addition & 0 deletions test-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"@embroider/test-setup": "^1.8.0",
"@glimmer/component": "^1.1.2",
"@glimmer/tracking": "^1.1.2",
"@tsconfig/ember": "^2.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like we should remove this from the package.json or from the ember-try scenario. I tend to remove it from the package.json in scope of the current test-app

Copy link
Author

Choose a reason for hiding this comment

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

See comment above.

"@types/ember": "^3.1.1",
"@types/ember-qunit": "^3.4.13",
"@types/ember-resolver": "^5.0.10",
Expand Down
2 changes: 1 addition & 1 deletion test-app/tests/acceptance/collection-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module('Acceptance | collection', function (hooks) {
setupApplicationTest(hooks);

test(`allows to traverse nodes while they don't exist`, async function (assert) {
await page.visit().numbers[2].click();
await page.visit().numbers[2]?.click();

assert.ok(1, 'exception is not thrown');
});
Expand Down
4 changes: 2 additions & 2 deletions test-app/tests/unit/-private/composition-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,11 @@ module('Unit | composition', function () {
getPageObjectDefinition({});
assert.true(false);
} catch (e) {
assert.strictEqual(e.toString(), 'Error: cannot get the page object definition from a node that is not a page object');
assert.strictEqual(e?.toString(), 'Error: cannot get the page object definition from a node that is not a page object');
}
});

module('all properties via compsition', function (hooks) {
module('all properties via composition', function (hooks) {
setupRenderingTest(hooks);

test('can alias through composition', async function (assert) {
Expand Down
14 changes: 7 additions & 7 deletions test-app/tests/unit/-private/properties/collection-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,13 @@ module('collection', function(hooks) {

let array = page.foo.toArray();
assert.equal(array.length, 2);
assert.equal(array[0].text, 'Lorem');
assert.equal(array[1].text, 'Ipsum');
assert.equal(array[0]?.text, 'Lorem');
assert.equal(array[1]?.text, 'Ipsum');

let proxyArray = page.foo.toArray();
assert.equal(proxyArray.length, 2);
assert.equal(proxyArray[0].text, 'Lorem');
assert.equal(proxyArray[1].text, 'Ipsum');
assert.equal(proxyArray[0]?.text, 'Lorem');
assert.equal(proxyArray[1]?.text, 'Ipsum');
});

test('produces an iterator for items', async function(this: TestContext, assert) {
Expand Down Expand Up @@ -548,7 +548,7 @@ module('collection', function(hooks) {
`);

assert.deepEqual(page.foo.filter((i) => i.isSpecial).map((i) => i.text), ['Lorem']);
assert.deepEqual(page.foo.filter((i) => i.isFoo as any).map((i) => i.text), []);
assert.deepEqual(page.foo.filter((i) => i['isFoo'] as any).map((i) => i.text), []);
});

test('filterBy works correctly', async function(this: TestContext, assert) {
Expand Down Expand Up @@ -578,7 +578,7 @@ module('collection', function(hooks) {
<span>Ipsum</span>
`);

assert.equal(page.foo[0].text, 'Lorem');
assert.equal(page.foo[1].text, 'Ipsum');
assert.equal(page.foo[0]?.text, 'Lorem');
assert.equal(page.foo[1]?.text, 'Ipsum');
});
});
2 changes: 1 addition & 1 deletion test-app/tests/unit/-private/properties/create-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ module('create', function () {
// @ts-expect-error violate types to check if it fails in weakly-typed envs
create('');
assert.true(false, 'should error');
} catch (e) {
} catch (e: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we have Error instead of any?

Copy link
Author

Choose a reason for hiding this comment

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

We can't use the type Error here, due to:

Catch clause variable type annotation must be 'any' or 'unknown' if specified.ts(1196)

This is an annoying issue with TypeScript, see a long discussion thread here. Alternatively, and probably more correct, we can type this as unknown.

assert.strictEqual(e.message, 'Definition can not be a string');
}
});
Expand Down
6 changes: 3 additions & 3 deletions test-app/tests/unit/-private/properties/fillable-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ module('fillable', function(hooks) {
foo: fillable()
});

await this.createTemplate(`<div class="scope">${this.template}</div>`);
await this.createTemplate(`<div class="scope">${this['template']}</div>`);

await page.foo(clue, expectedText);

Expand All @@ -80,10 +80,10 @@ module('fillable', function(hooks) {
foo: fillable()
});

this.attrName = attrName;
this[attrName] = attrName;

await this.createTemplate(`<div class="scope">
<div contenteditable ${this.attrName}="clue"></div>
<div contenteditable ${this[attrName]}="clue"></div>
</div>`);

await page.foo(clue, expectedText);
Expand Down
4 changes: 2 additions & 2 deletions test-app/tests/unit/-private/properties/getter-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ module('getter', function(hooks) {
page.foo;
assert.true(false);
} catch (e) {
assert.strictEqual(e.toString(), `Error: Argument passed to \`getter\` must be a function.
assert.strictEqual(e?.toString(), `Error: Argument passed to \`getter\` must be a function.

PageObject: \'page.foo\'`)
}
Expand All @@ -98,7 +98,7 @@ PageObject: \'page.foo\'`)
page.foo;
assert.true(false);
} catch (e) {
assert.strictEqual(e.toString(), `Error: custom error message
assert.strictEqual(e?.toString(), `Error: custom error message

PageObject: \'page.foo\'`)
}
Expand Down
2 changes: 1 addition & 1 deletion test-app/tests/unit/-private/properties/visitable-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ module('visitable', function(hooks) {

assert.false(true, 'visit should have failed');
} catch (e) {
assert.strictEqual(e.toString(), `Error: Failed to visit URL '/non-existing-url/value'
assert.strictEqual(e?.toString(), `Error: Failed to visit URL '/non-existing-url/value'

PageObject: 'page.foo(\"[object Object]\")'
Selector: '.scope'`);
Expand Down
51 changes: 11 additions & 40 deletions test-app/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,18 @@
{
"extends": "@tsconfig/ember/tsconfig.json",
"compilerOptions": {
"target": "es2020",
"allowJs": true,
"moduleResolution": "node",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to make sure we still support the moduleResolution: "node". Also, I think leaving this test app in a "legacy" state makes sense until we decide to increase the bottom line of the support matrix.

Maybe a better way to test would be just to create a new cutting-edge test app, with just a single dummy test that imports some ember-cli-page-object module + tsc as a test.

...Or, alternatively, we could create a "legacy" test app with that dummy test + tsc, but then I think it makes sense to upgrade the current test app to the latest, which seems to be just more work to do 🤔

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

The @tsconfig/ember 2.0.0 version has moduleResolution set to node (see here) so there's not really a change in that respect. It only changes the value to bundler with the 3.0.0 version (here).

Thoughts?

"allowSyntheticDefaultImports": false,
"noImplicitAny": true,
"noImplicitThis": true,
"alwaysStrict": true,
"strictNullChecks": true,
"strictPropertyInitialization": true,
"noFallthroughCasesInSwitch": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"noImplicitReturns": true,
"noEmitOnError": true,
"noEmit": true,
"inlineSourceMap": true,
"inlineSources": true,
// The combination of `baseUrl` with `paths` allows Ember's classic package
// layout, which is not resolvable with the Node resolution algorithm, to
// work with TypeScript.
"baseUrl": ".",
"module": "es6",
"experimentalDecorators": true,
"paths": {
"test-app/tests/*": [
"tests/*"
],
"test-app/*": [
"app/*"
],
"ember-cli-page-object": [
"addon"
],
"ember-cli-page-object/*": [
"addon/*"
],
"ember-cli-page-object/test-support": [
"addon-test-support"
],
"ember-cli-page-object/test-support/*": [
"addon-test-support/*"
],
"*": [
"types/*"
]
"test-app/tests/*": ["tests/*"],
"test-app/*": ["app/*"],
"ember-cli-page-object": ["addon"],
"ember-cli-page-object/*": ["addon/*"],
"ember-cli-page-object/test-support": ["addon-test-support"],
"ember-cli-page-object/test-support/*": ["addon-test-support/*"],
"*": ["types/*"]
}
},
"include": [
Expand Down
Loading