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

Assure gts works #206

Merged
merged 3 commits into from
Sep 25, 2023
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
13 changes: 12 additions & 1 deletion AUTHORING_LIBRARIES.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,21 @@ If you've done ESM in node, this should feel familiar, and we can be be consiste
import { AnotherThing } from './path/to/file.js';
```

Generally, import:
- gjs with `./path/to/file.gjs`
- gts with `./path/to/file.gts`
- js with `./path/to/file.js`
- ts with `./path/to/file.ts`
- hbs with `./path/to/file.js` or `./path/to/file`

A couple caveats with older, co-located components,
- for `.hbs` / template-only components, the import path is still `.js`, because we transform it.
- for `.hbs` / template-only components, no extension is needed, but the js extension can be used.
- for co-located components, where the template is in a separate `.hbs` file, you may not import that `.hbs` file directly, because it is merged in with the associated `.js` or `.ts` file.

For consumers of your library, they will not need to worry about the extensions, because:
- rollup compiles away the implementation details (non-js modules)
- package.json#exports declares what is importable under what path, and maps non-extension imports to files with extensions


## CSS

Expand Down
9 changes: 6 additions & 3 deletions files/__addonLocation__/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@
"@babel/plugin-transform-class-static-block": "^7.20.0",
"@babel/runtime": "^7.17.0",
"@embroider/addon-dev": "^4.1.0",<% if (typescript) { %>
"@glint/core": "^1.0.2",
"@glint/environment-ember-loose": "^1.0.2",
"@glint/template": "^1.0.2",
"@glint/core": "^1.2.0",
"@glint/environment-ember-loose": "^1.2.0",
"@glint/environment-ember-template-imports": "^1.2.0",
"@glint/template": "^1.2.0",
"@tsconfig/ember": "^2.0.0",
"@types/ember": "^4.0.4",
"@types/ember__object": "^4.0.6",
Expand All @@ -64,6 +65,8 @@
"@types/ember__array": "^4.0.4",
"@types/ember__error": "^4.0.3",
"@types/ember__component": "^4.0.14",
"@types/ember__modifier": "^4.0.6",
"@types/ember__helper": "^4.0.3",
"@types/ember__routing": "^4.0.13",
"@typescript-eslint/eslint-plugin": "^6.7.2",
"@typescript-eslint/parser": "^6.7.2",<% } %>
Expand Down
2 changes: 1 addition & 1 deletion files/__addonLocation__/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"unpublished-development-types/**/*"
],
"glint": {
"environment": "ember-loose"
"environment": ["ember-loose", "ember-template-imports"]
},
"compilerOptions": {
"allowJs": true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import templateOnly from '@ember/component/template-only';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file is needed so that Glint knows what type the hbs file is.

In TS, we cannot have bare-hbs-file-components.


export default templateOnly();
40 changes: 40 additions & 0 deletions tests/helpers/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,46 @@ export async function createTmp() {
return tmpDirPath;
}

const ROLLUP_HASH = /[a-f0-9]{8}/

/**
* Filters out rollup-hashes from a list of files.
*
* When there are private files, rollup will add some
* hash to the emitted file path,
* such as:
* template-only-ab2e7769.js.map
*
* These hashes have predictable length, so we can have
* a fairly narrow matcher to remove them.
*/
export function withoutHashes(names: string[]) {
return names.filter(name => !ROLLUP_HASH.test(name));
}

/**
* Filters out a list of files, keeping only the rollup-emitted hashed files
*
* When there are private files, rollup will add some
* hash to the emitted file path,
* such as:
* template-only-ab2e7769.js.map
*
* These hashes have predictable length, so we can have
* a fairly narrow matcher to remove them.
* This a
*/
export function hashesOnly(names: string[]) {
return names.filter(name => ROLLUP_HASH.test(name));
}

export function splitHashedFiles(names: string[]) {
return {
hashed: hashesOnly(names),
unhashed: withoutHashes(names),
};
}

/**
* Abstraction for install, as the blueprint supports multiple package managers
*/
Expand Down
22 changes: 17 additions & 5 deletions tests/smoke-tests/--typescript.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
AddonHelper,
assertGeneratedCorrectly,
dirContents,
splitHashedFiles,
SUPPORTED_PACKAGE_MANAGERS,
} from '../helpers.js';

Expand Down Expand Up @@ -71,13 +72,12 @@ for (let packageManager of SUPPORTED_PACKAGE_MANAGERS) {

expect(buildResult.exitCode).toEqual(0);

let distContents = (await dirContents(distDir)).filter(
// these files have a hash that changes based on file contents
(distFile) => !distFile.startsWith('_rollupPluginBabelHelpers')
);
let distContents = splitHashedFiles(await dirContents(distDir));
let declarationsContents = await dirContents(declarationsDir);

expect(distContents).to.deep.equal([


expect(distContents.unhashed).to.deep.equal([
'_app_',
'components',
'index.js',
Expand All @@ -86,6 +86,18 @@ for (let packageManager of SUPPORTED_PACKAGE_MANAGERS) {
'template-registry.js.map',
]);

expect(distContents.hashed.length).toBe(4);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it 4, and is this necessarily the case? Or are we assuming rollup implementation details here, and potentially making the test brittle?

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Sep 25, 2023

Choose a reason for hiding this comment

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

rollupSomething js + map
template-only js + map

Previously the rollup files were filtered out

I do not believe the test would become brittle (all 4 files have hashes in their names, so I can't directly check what their file names are)

expect(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if anyone knows of a more ergonomic way to do this, I'm all ears

distContents.hashed
.filter(file => file.includes('_rollup'))
.map(file => file.split('.js')[1])
).to.deep.equal(["", '.map'], 'the rollup helpers are emitted with a source map');
expect(
distContents.hashed
.filter(file => file.includes('template-only'))
.map(file => file.split('.js')[1])
).to.deep.equal(["", '.map'], 'the template-only component is emitted with a source map');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these additional assertions related to gts support? Or why are they in this PR?

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 template-only file, it was previously un-asserted.

Since the import path changed (by adding an extension), I thought it might be good to assure the template-only component is in the dist output.

🤷


expect(declarationsContents).to.deep.equal([
'components',
'index.d.ts',
Expand Down