Move off rollup-plugin-ts #136

NullVoxPopuli · 2023-07-03T15:13:05Z

Resolves: #135, #112, and maybe #133

Because tsc/glint and rollup each have their own cleanup phase, we can't use reliably the same dist directory anymore. So, for type-declarations, I've chosen the creatively named "dist-types" for where the declarations will go and configured in the tsconfig.json to output the declarations there.

We originally(?) had a setup similar to this, but had wanted to have type-declaration generation combined with the rollup build. @rollup/plugin-typescript and rollup-plugin-ts both offer this, but rollup-plugin-ts does way too much, and has been causing us problems. @rollup/plugin-typescript doesn't integrate with Glint/gjs/gts, so that's not an option either. (See #135, and related links for more context).

Additionally, the approach here, with manually specifying extensions looks annoying, but it's flexible, and I have an addon using gjs/gts with this approach already.

simonihmig · 2023-07-03T15:54:49Z

For v1 addons (since merging ember-cli/ember-cli#10283) we have dist (used by Ember CLI, not published) and declarations for the types. Should we align to this here?

…ith newer ember

NullVoxPopuli · 2023-07-03T16:00:16Z

probably, yeah.

I'll make the change once I can figure out how to get CI to a green state.

Right now it looks like I need to first rip out the @types/* packages

simonihmig · 2023-07-03T16:51:17Z

The failing test seems unrelated to @types AFAICT, and quite close to passing though?

NullVoxPopuli · 2023-07-03T17:09:01Z

That is different from what I saw locally!
That is quite exciting.
Also, at some point, I should figure out how to get CI / local not able to be different

NullVoxPopuli · 2023-07-03T17:14:34Z

oh, and look at that, it's green! yay!

ijlee2 · 2023-07-03T17:29:22Z

Before merging the pull request, I think it'd be a good idea to see the suggested change in action. That is, check that the code change works on existing v2 addons (at least two---one with Glint and another without). The pull requests for those addons could serve as a migration guide. What do you think?

On a related note, will there be an announcement from the Embroider team to v2 addon authors that they should migrate away from rollup-plugin-ts soon?

NullVoxPopuli · 2023-07-03T18:02:00Z

that they should migrate away

@ijlee2 Personally, I don't see that v2 addon authors, v1 addon authors, and app authors would have to adhere to whatever is in the blueprint. If rollup-plugin-ts is working for folks, then they shouldn't feel like they need to change anything.

Also, we have tests that generate addons and ensure that they are buildable 🎉

announcement from the Embroider team to v2 addon authors

This is typically the blueprint changelog, yeah?

files/__addonLocation__/.eslintignore

files/__addonLocation__/.prettierignore

simonihmig

The changes look great to me!

However I was thinking to maybe try out the changes in some real-world addons, to make sure everything DX-wise works as expected...

NullVoxPopuli · 2023-07-04T10:58:31Z

I did one yesterday: https://github.com/universal-ember/ember-primitives/blob/main/ember-primitives/package.json#L17
Tho, still using dist-types instead of declarations

NullVoxPopuli · 2023-07-04T12:04:09Z

Here are two more, created directly from this blueprint:

❯ EMBER_CLI_PNPM=true npx ember-cli@beta addon no-rollup-plugin-ts-js --pnpm --addon-only -b ../OpenSource/emberjs/addon-blueprint/


❯ EMBER_CLI_PNPM=true npx ember-cli@beta addon no-rollup-plugin-ts-ts --pnpm --addon-only -b ../OpenSource/emberjs/addon-blueprint/ --typescript

This is the only thing I noticed with the TS blueprint,

.
└─┬ @glint/environment-ember-loose 1.0.2
  ├── ✕ unmet peer @types/ember__array@^4.0.2: found 4.0.0
  ├── ✕ unmet peer @types/ember__component@^4.0.10: found 4.0.0
  ├── ✕ unmet peer @types/ember__controller@^4.0.2: found 4.0.0
  ├── ✕ unmet peer @types/ember__object@^4.0.4: found 4.0.0
  └── ✕ unmet peer @types/ember__routing@^4.0.11: found 4.0.0

which is causing issues (types don't build due to errors -- but that's a separate issue from this PR -- the minimum versions in the package.json are too low.

I did see this time when generating these repos that we need to have the "no clear screen" flag turned on for rollup and tsc

NullVoxPopuli · 2023-07-04T12:07:11Z

Looks like Glint doesn't have a way to not clear the screen in watch mode -- I'll open an issue over there

typed-ember/glint#597

Update the addon boilerplate to align with embroider-build/addon-blueprint#136.

Co-authored-by: Simon Ihmig <[email protected]>

dfreeman · 2023-07-05T08:25:05Z

files/__addonLocation__/package.json

    "lint": "concurrently 'npm:lint:*(!fix)' --names 'lint:'",
    "lint:fix": "concurrently 'npm:lint:*:fix' --names 'fix:'",
    "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",<% if (typescript) { %>
-    "lint:types": "glint",<% } %>
-    "start": "rollup --config --watch",
+    "lint:types": "glint",


Why does this sometimes use --build and other times not? Are you using --build here because it implies --declaration?

As we've discussed in the past, --build is for orchestrating composite multi-project builds by traversing references, and has nontrivial overhead in terms of the machinery it sets up. If you're going to use it, you should use it consistently for all glint/tsc commands, but in this case it doesn't seem like it should be necessary.

Good callout, I missed that!

I tested this in my addon, and came up with the changes in this commit:

remove --build

remove all compilerOptions except for declarationDir. The other don't seem to be needed AFAICT, maybe implied by -d?

added -d to the build:types script. Having "declaration": true alone does not seem to work, -d is strictly needed for glint to output declaration files, is that right?

The other don't seem to be needed AFAICT, maybe implied by -d?

They may be set to reasonable defaults in @tsconfig/ember

Having "declaration": true alone does not seem to work, -d is strictly needed for glint to output declaration files, is that right?

It's arguably a bug in Glint that we require you to run with --declaration to emit .d.ts files. I think the reason we landed there is that there's some hazy gray area in terms of what it means for us to match tsc's own behavior, since we effectively hard-code noEmit: true, which implies certain things about how other config like declarations behaves.

Regardless, you're correct that you have to use the --declaration/-d flag for the Glint CLI to emit .d.ts files today, even if we should potentially revisit that in Glint at some point.

Looking at that commit you linked, I think you also probably want -d in your start:types script so that that will update your .d.ts files as your sources change. Otherwise looks reasonable to me! 👍

mansona

just a minor suggestion 😂

mansona · 2023-07-05T09:04:08Z

files/__addonLocation__/package.json

+  "scripts": {<% if (typescript) { %>
+    "build": "concurrently 'npm:build:*'",
+    "build:js": "rollup --config",
+    "build:types": "glint --build", <% } else { %>
+    "build": "rollup --config", <% } %>


What do you think of keeping concurrently for both TS and JS builds? that way we can just add the build:types in case of typescript

Just an idea

idk, if we keep each convenience thing for TS in the JS version of the addon, we'd probably have a lot less conditionals all over the place.

Since this is blocking other work, how do you feel about discussing in another pr?

don't think of any of my comments as a blocker 👍 they are only minor suggestions that are more helpful as DX for us than anything else

Personally, i'm quite used to bending myself in to a pretzel so that consumers have a nice experience 🙃😅

mansona · 2023-07-05T09:05:05Z

files/__addonLocation__/package.json

+    "start": "concurrently 'npm:start:*'",
+    "start:js": "rollup --config --watch --no-watch.clearScreen",
+    "start:types": "glint --build --watch",<% } else { %>
+    "start": "rollup --config --watch", <% } %>


same as above comment re: concurrently and only adding or removing the start:types based on the flag

ijlee2 · 2023-07-05T09:43:14Z

files/__addonLocation__/gitignore

+# npm/pnpm/yarn pack output
+*.tgz


@NullVoxPopuli Can you explain to me how you had encountered the need to ignore *.tgz?

When I ran the publish --dry-run command, I didn't see any file with the .tgz extension so I feel like we wouldn't need lines 9-10.

If you run pnpm pack (for example, to verify a package would publish as you expect), you get a tgz file in your addon directory - we don't want that comitted.

Ah, ok. Thanks for providing more context.

files/__addonLocation__/package.json

ijlee2 · 2023-07-05T09:44:54Z

files/__addonLocation__/package.json

-    "start": "rollup --config --watch",
+    "lint:types": "glint",
+    "start": "concurrently 'npm:start:*'",
+    "start:js": "rollup --config --watch --no-watch.clearScreen",


I didn't encounter the need for --no-watch.clearScreen and would recommend keeping commands simple.

Suggested change

"start:js": "rollup --config --watch --no-watch.clearScreen",

"start:js": "rollup --config --watch",

The above matches the command for the JavaScript case.

Without this, both rollup and glint (tsc) fight for clearing the screen.
To prevent accidentally hiding issues, we need the screen to not clear on (re)build (which is what the flag does).

I have an issue for myself to investigate for the glint side of this: typed-ember/glint#597 (comment)

Sounds good, thanks.

files/__addonLocation__/package.json

files/__addonLocation__/rollup.config.mjs

ijlee2 · 2023-07-05T10:00:08Z

@NullVoxPopuli Nice work. In 4 addons (though none without Glint), I checked that we can remove rollup-plugin-ts as suggested by this PR:

Co-authored-by: Isaac Lee <[email protected]>

NullVoxPopuli · 2023-07-05T11:21:25Z

Nice work.

It's a team effort! I super appreciate the level of depth you all put in to reviewing this PR! ♥️

ijlee2 · 2023-07-05T12:17:28Z

files/__addonLocation__/package.json

-  "scripts": {
-    "build": "rollup --config",
+  "scripts": {<% if (typescript) { %>
+    "build": "concurrently 'npm:build:*'",


suggestion (optional)

When using concurrently for build and start, we could add --names like we do for lint.

- "build": "concurrently 'npm:build:*'", + "build": "concurrently 'npm:build:*' --names 'build:'",

# Before [types] > [email protected] build:types [types] > glint --build [types] [js] [js] > [email protected] build:js [js] > rollup --config [js]

# After [build:types] > [email protected] build:types [build:types] > glint --build [build:types] [build:js] [build:js] > [email protected] build:js [build:js] > rollup --config [build:js]

I kinda like the more concise output here. I will concede to others tho, if that's what folks want

Move off rollup-plugin-ts

ee11933

NullVoxPopuli added the bug Something isn't working label Jul 3, 2023

NullVoxPopuli added 3 commits July 3, 2023 11:22

Add .ts to extensions

98a0b61

Update gitignore

c0e25e0

Update typescript test

b1efe10

TS build fails because the @types/* dependencies are not compatible w…

2f07e9b

…ith newer ember

NullVoxPopuli mentioned this pull request Jul 3, 2023

Use built in types from ember-source instead of the types-packages #118

Draft

Update file assertion

ded3453

NullVoxPopuli marked this pull request as ready for review July 3, 2023 17:14

Rename dist-types to declarations

8e84e0c

simonihmig reviewed Jul 3, 2023

View reviewed changes

files/__addonLocation__/.eslintignore Outdated Show resolved Hide resolved

files/__addonLocation__/.prettierignore Outdated Show resolved Hide resolved

simonihmig mentioned this pull request Jul 3, 2023

Cannot find module '@babel/plugin-proposal-object-rest-spread' #133

Closed

Include hidden files in the fild and replace

a285f08

NullVoxPopuli mentioned this pull request Jul 3, 2023

Update for ember-cli --typescript changes #137

Merged

NullVoxPopuli requested a review from simonihmig July 3, 2023 20:57

simonihmig reviewed Jul 4, 2023

View reviewed changes

Add --no-watch.clearScreen to rollup's start

6145298

NullVoxPopuli mentioned this pull request Jul 4, 2023

Option to not clear the screen when in watch mode? typed-ember/glint#597

Closed

simonihmig added a commit to simonihmig/ember-stargate that referenced this pull request Jul 4, 2023

Drop rollup-plugin-ts, use updated addon blueprint

0169b48

Update the addon boilerplate to align with embroider-build/addon-blueprint#136.

simonihmig mentioned this pull request Jul 4, 2023

Drop rollup-plugin-ts, use updated addon blueprint simonihmig/ember-stargate#655

Merged

NullVoxPopuli and others added 2 commits July 4, 2023 14:06

Update files/__addonLocation__/rollup.config.mjs

23d9ed5

Co-authored-by: Simon Ihmig <[email protected]>

Update files/__addonLocation__/package.json

db510d7

Co-authored-by: Simon Ihmig <[email protected]>

NullVoxPopuli mentioned this pull request Jul 4, 2023

Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

Closed

simonihmig approved these changes Jul 5, 2023

View reviewed changes

dfreeman reviewed Jul 5, 2023

View reviewed changes

mansona reviewed Jul 5, 2023

View reviewed changes

This was referenced Jul 5, 2023

Remove rollup-plugin-ts ijlee2/ember-container-query#203

Merged

Remove rollup-plugin-ts ijlee2/embroider-css-modules#89

Merged

ijlee2 reviewed Jul 5, 2023

View reviewed changes

files/__addonLocation__/package.json Outdated Show resolved Hide resolved

ijlee2 reviewed Jul 5, 2023

View reviewed changes

files/__addonLocation__/package.json Outdated Show resolved Hide resolved

ijlee2 reviewed Jul 5, 2023

View reviewed changes

files/__addonLocation__/rollup.config.mjs Outdated Show resolved Hide resolved

ijlee2 mentioned this pull request Jul 5, 2023

Remove rollup-plugin-ts ijlee2/ember-codemod-v1-to-v2#51

Merged

NullVoxPopuli and others added 3 commits July 5, 2023 07:14

Update files/__addonLocation__/package.json

01f8f24

Co-authored-by: Isaac Lee <[email protected]>

Update files/__addonLocation__/package.json

b650e97

Co-authored-by: Isaac Lee <[email protected]>

Update files/__addonLocation__/rollup.config.mjs

5376caf

Co-authored-by: Isaac Lee <[email protected]>

ijlee2 reviewed Jul 5, 2023

View reviewed changes

Use glint -d

c6da1e6

NullVoxPopuli merged commit 22eb665 into main Jul 5, 2023

NullVoxPopuli deleted the get-off-rollup-plugin-ts branch July 5, 2023 15:45

ijlee2 mentioned this pull request Jul 5, 2023

build:types script doesn't work #139

Closed

simonihmig mentioned this pull request Jul 5, 2023

Skip .gitignore in existing monorepo #129

Merged

ijlee2 mentioned this pull request Jul 6, 2023

glint --declaration fails to create declaration files when @glint-ignore, @glint-expect-error, or @glint-nocheck is used typed-ember/glint#599

Open

simonihmig added enhancement New feature or request and removed bug Something isn't working labels Jul 6, 2023

vscav mentioned this pull request Sep 1, 2023

Expose template registry and drop rollup-plugin-ts qonto/ember-lottie#194

Merged

6 tasks

kevinkucharczyk mentioned this pull request Sep 28, 2023

Remove rollup plugin ts, use @glimmer imports customerio/ember-provide-consume-context#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move off rollup-plugin-ts #136

Move off rollup-plugin-ts #136

NullVoxPopuli commented Jul 3, 2023 •

edited

Loading

simonihmig commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

simonihmig commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

ijlee2 commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

simonihmig left a comment

NullVoxPopuli commented Jul 4, 2023

NullVoxPopuli commented Jul 4, 2023

NullVoxPopuli commented Jul 4, 2023 •

edited

Loading

dfreeman Jul 5, 2023 •

edited

Loading

simonihmig Jul 5, 2023

dfreeman Jul 5, 2023 •

edited

Loading

mansona left a comment

mansona Jul 5, 2023

NullVoxPopuli Jul 5, 2023

mansona Jul 5, 2023

NullVoxPopuli Jul 5, 2023

mansona Jul 5, 2023

ijlee2 Jul 5, 2023

NullVoxPopuli Jul 5, 2023

ijlee2 Jul 5, 2023

ijlee2 Jul 5, 2023

NullVoxPopuli Jul 5, 2023

ijlee2 Jul 5, 2023

ijlee2 commented Jul 5, 2023

NullVoxPopuli commented Jul 5, 2023

ijlee2 Jul 5, 2023 •

edited

Loading

NullVoxPopuli Jul 5, 2023

	"start:js": "rollup --config --watch --no-watch.clearScreen",
	"start:js": "rollup --config --watch",

Move off rollup-plugin-ts #136

Move off rollup-plugin-ts #136

Conversation

NullVoxPopuli commented Jul 3, 2023 • edited Loading

simonihmig commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

simonihmig commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

ijlee2 commented Jul 3, 2023

NullVoxPopuli commented Jul 3, 2023

simonihmig left a comment

Choose a reason for hiding this comment

NullVoxPopuli commented Jul 4, 2023

NullVoxPopuli commented Jul 4, 2023

NullVoxPopuli commented Jul 4, 2023 • edited Loading

dfreeman Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreeman Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

mansona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijlee2 commented Jul 5, 2023

NullVoxPopuli commented Jul 5, 2023

ijlee2 Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Jul 3, 2023 •

edited

Loading

NullVoxPopuli commented Jul 4, 2023 •

edited

Loading

dfreeman Jul 5, 2023 •

edited

Loading

dfreeman Jul 5, 2023 •

edited

Loading

ijlee2 Jul 5, 2023 •

edited

Loading