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

Convert addons to template tag and gts #246

Merged
merged 4 commits into from
Aug 7, 2023
Merged

Convert addons to template tag and gts #246

merged 4 commits into from
Aug 7, 2023

Conversation

ynotdraw
Copy link
Contributor

@ynotdraw ynotdraw commented Aug 3, 2023

🚀 Description

Converts toucan-core and toucan-form to use the <template> tag and gts file extension. A couple of things worth mentioning:

Massive PR

Unfortunately this PR is massive; however, not much code was actually changed. It was mostly creating a gts file, then copying the hbs file into the <template> and then adding any helper imports needed. While I was in the code I did a few things like added missing JSdocs or renamed things, but everything else stayed put.

Tailwind CSS Autocomplete

NOTE: This only applies to you if you use VSCode.

In VS code, it does not recognize how to format gjs/gts class names by default. Due to that, you need to help the extension out by telling it which language glimmer-ts is. We map it to "html".

Go to the settings for the TailwindCSS extension and add the following:

Screenshot_2023-08-03_at_8 57 01_AM

prettier-plugin-tailwindcss

The downside with gjs/gts is that it appears our prettier-plugin-tailwindcss rule no longer works. This rule would auto-sort the class names for us based on Tailwind's rules. I believe we may need to update the prettier glimmer plugin, but I asked on the Ember Discord for clarification. I could also be missing something. At the time I was thinking maybe we needed something like above to help tell the underlying library how to handle these files, but since it's prettier rather than an extension, we don't appear to have the correct hooks for that configuration.

I suppose the good news would be that once the underlying issue is resolved, it should all just work in a future prettier release.

https://discord.com/channels/480462759797063690/486516209873846292/1136646656977289236


🔬 How to Test


📸 Images/Videos of Functionality

N/A

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

🦋 Changeset detected

Latest commit: 0e817b6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@crowdstrike/ember-toucan-core Patch
@crowdstrike/ember-toucan-form Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Preview URLs

Env: preview
Docs: https://7cda0b2c.ember-toucan-core.pages.dev

@ynotdraw ynotdraw marked this pull request as ready for review August 3, 2023 16:37
Comment on lines +10 to +15
{
files: '*.gjs',
options: {
parser: 'ember-template-tag',
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we could axe this as we will probably only use gts?

@@ -36,7 +36,7 @@
"@ember/string": "^3.0.1",
"@ember/test-helpers": "^3.1.0",
"@embroider/compat": "^2.0.0",
"@embroider/core": "^2.0.0",
"@embroider/core": "2.0.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to pin this for now due to an error I was seeing. The task to upgrade us to the latest embroider stuff is at #210

The error was:

docs-app:build: $TMPDIR/embroider/6cc172/node_modules/.pnpm/file+packages+ember-toucan-core_@[email protected]_@[email protected]_@e_y4sbboigrbv5roskhqf7pymkbu/node_modules/@crowdstrike/ember-toucan-core/dist/components/form/fields/textarea.js: Scope objects for `precompileTemplate` may only contain direct references to in-scope values, e.g. { Field } or { Field: Field }
docs-app:build:   115 |   `, {
docs-app:build:   116 |   strictMode: true,
docs-app:build: > 117 |   scope: () => ({
docs-app:build:       |   ^
docs-app:build:   118 |     Field: ToucanFormFieldComponent,
docs-app:build:   119 |     hash,
docs-app:build:   120 |     field,

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment somewhere? One possibility:

`"devDependencyComments": {
  "@embroider/core": "[your comment]"
},`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dig it - added!

@@ -108,7 +108,7 @@
"postcss": "^8.4.17",
"postcss-import": "^15.0.0",
"postcss-loader": "^7.0.1",
"prettier": "^2.8.3",
"prettier": "^3.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to update prettier so that the prettier template plugin would work properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this one as we have one in the root. This was likely left over from when we moved from the single-package to the multiple-package format.

Comment on lines +76 to +77
"@types/ember__helper": "^4.0.2",
"@types/ember__modifier": "^4.0.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With gjs/gts we need to import helpers like on, fn, and hash explicitly. By adding these dependencies, it allows us to do so (see below!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to trace helpers back to import statements is perhaps my favorite part of gjs/gts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree 🎉

import { action } from '@ember/object';

import Check from '../../../../../-private/icons/check';

interface ToucanFormAutocompleteOptionControlComponentSignature {
interface ToucanCoreAutocompleteOptionComponentSignature {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these -private components, I updated the prefix to ToucanCore*

@@ -76,9 +77,9 @@ export default class Button extends Component<ButtonSignature> {

assert(
`Invalid variant for Button: '${variant}' (allowed values: [${VALID_VARIANTS.join(
', '
', ',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this change was due to the latest version of prettier where trailing commas are now the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured I'd do a patch upgrade just in case things would break consumers for some strange reason 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency I took these settings from ember-headless-form.

@@ -36,7 +36,7 @@
"@ember/string": "^3.0.1",
"@ember/test-helpers": "^3.1.0",
"@embroider/compat": "^2.0.0",
"@embroider/core": "^2.0.0",
"@embroider/core": "2.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment somewhere? One possibility:

`"devDependencyComments": {
  "@embroider/core": "[your comment]"
},`

Comment on lines +76 to +77
"@types/ember__helper": "^4.0.2",
"@types/ember__modifier": "^4.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to trace helpers back to import statements is perhaps my favorite part of gjs/gts.

@@ -26,11 +27,15 @@ export default {
// not everything in publicEntrypoints necessarily needs to go here.
addon.appReexports(['components/**/*.js']),

// compile <template> tag into plain JS
glimmerTemplateTag({ preprocessOnly: true }),
Copy link
Contributor

@clintcs clintcs Aug 7, 2023

Choose a reason for hiding this comment

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

Off the top of your head, any idea what this compiles to when there's a backing class? A template class method maybe?

Copy link
Contributor Author

@ynotdraw ynotdraw Aug 7, 2023

Choose a reason for hiding this comment

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

I'm honestly not sure but Preston would know! A bit more info on https://github.com/ember-template-imports/ember-template-imports/blob/master/src/babel-plugin.js#L6-L55, but that's as far as I'll probably dig in 😂

@ynotdraw ynotdraw merged commit 04419d3 into main Aug 7, 2023
14 checks passed
@ynotdraw ynotdraw deleted the template-tag-gts branch August 7, 2023 20:12
@github-actions github-actions bot mentioned this pull request Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants