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

Creating a new app or addon with the --typescript flag overrules the use of the --skip-npm flag #10045

Closed
bertdeblock opened this issue Oct 6, 2022 · 9 comments · Fixed by #10283

Comments

@bertdeblock
Copy link

bertdeblock commented Oct 6, 2022

I.e. all node modules are installed regardless of if the user passed in the --skip-npm flag.
Probably because we use addAddonToProject to add the ember-cli-typescript addon to the generated app or addon.

@simonihmig
Copy link
Contributor

Yes, but I think that's inevitable. See my notes in the PR description: #9972.

@bertdeblock
Copy link
Author

The alternative I had in mind was:

  • Already add ember-cli-typescript to the package.json file in the blueprint
  • If the --skip-npm flag is not passed, run the default blueprint of ember-cli-typescript after running the npm-install task
  • When the --skip-npm flag is passed, display a warning that the user should manually run ember g ember-cli-typescript

I think if someone specifically uses the --skip-npm flag, this feels like an okay thing to do, to avoid installing dependencies.

@simonihmig
Copy link
Contributor

I see. Sounds reasonable in a way, but...

I think if someone specifically uses the --skip-npm flag, this feels like an okay thing to do, to avoid installing dependencies.

Well, either way we somehow break the expectations of the user. When passing both --typescript and --skip-npm, ideally the user should be able to expect getting an app that is fully set up for TS, just has no node_modules. Currently we break the expectation by installing dependencies. In your proposal, we would break the expectations by not giving the user a fully set up app (i.e. missing tsconfig.json). For me what we have now feels like the lesser evil!? 🤷

There is probably a way to not break expectations at all, either by incorporating all the things the e-c-ts blueprint does into our own (which fwiw goes against the related RFC), or add e-c-ts as a dependency of ember-cli itself (so we don't need to install it later), and run its blueprint from there somehow. Not sure if this issue is worth the trouble?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 3, 2023

from my experience using --typescript, the fact that --skip-npm is ignore is hugely problematic. This is in CLI tools, where I'm expecting to do as much statically as possible, as well as just normal usage, where I want pnpm used, and I don't want npm or yarn thrashing my disk. (I imagine this is much less of a problem for folks "just wanting to try an ember app with TS", and don't mind defaults)

I know the built-in types are getting pretty close to being out of "preview", so I'd like to propose we

  • remove ember-cli-typescript entirely
  • the --typescript flag adds tsconfig.json, adds TS-compilation to the ember-cli-babel config in ember-cli-build.js, and generates the types` folder with the import of the built-in types. This is fastest for everyone involved, I think as we're overall doing less file and network i/o.

@simonihmig
Copy link
Contributor

I feel you, and I was not happy about it either when implementing this, having caused (or uncovered) other issues as well (slow tests, things to fix in Ember CLI itself). But as I said above, we have an RFC from the TS core team that explicitly prescribed using e-c-ts and not getting rid of it (at this point). So I sticked to that...

Doesn't mean we cannot revisit this decision, but I think we need to involve the TS folks for this? I believe there were also initiatives planned to lay out how Ember+TS without e-c-ts would look like. So that should be accounted for. /cc @chriskrycho @dfreeman @jamescdavis

@chriskrycho
Copy link
Contributor

We don't have to stick with the exact flow designed in 0800, FWIW. One major motivator for the new stages process was that this kind of experience gained during the process of implementing it can and often should be grounds for revising the RFC. I’ll write up a proposed alternative flow here in a bit, and we can revise RFC 0800 with it!

@chriskrycho
Copy link
Contributor

Here’s the rough outline of what we need to do:

  1. Revise the RFC to say basically the opposite of what it currently does: we should move the functionality over from ember-cli-typescript into ember-cli and ember-cli-babel as appropriate. The core blueprint functionality should live in ember-cli. The transpilation for apps and classic addons should live in ember-cli-babel, as it actually already does.

    Eventually we will be able to drop the “handshake” ember-cli-babel does today; as part of this flow we’ll want it to switch to a no-op that informs that ember-cli-typescript is not necessary with the appropriate versions of ember-cli. (We will need to coordinate turning that on with having both versions out.)

  2. Write an RFC to sunset the rest of ember-cli-typescript in favor of directly using tsc or glint to type check and to emit type definitions. @hmajoros was looking into writing this RFC, since he just rolled out exactly that flow on the flagship app at LinkedIn (it works great!), so I will follow up with him and see if he is able to do it sooner rather than later; if not, we can guide someone else on what all it should cover, including transition paths etc.

    The one open question I have (and I will defer to @dfreeman on) is: out of the box, should we wire up tsc or glint? I am broadly inclined to agree with one part of a comment from @NullVoxPopuli, that we “may as well set up Glint as well, since it's nearly 1.0”

Net, we want to replace this bullet in the original RFC—

Install Ember TypeScript integration tooling, today consisting simply of ember-cli-typescript and its generators.

—with something shaped roughly like:

  • Set up TypeScript integration (migrated from ember-cli-typescript):
    • Create tsconfig.json files and generate their compilerOptions.paths in ember-cli.
    • Configure ember-cli-babel to include the TypeScript transform
    • Set up packages with scripts to do type checking and, in the case of v1 addons, emitting type declarations using tsc or glint.

On ember-cli-babel, it occurs to me that we can probably just enable that transform by default at this point? (Or maybe we already did?)


Process-wise, these are CLI and TS team concerns, so we just need alignment between our two teams and good communications about them to everyone else (and the usual FCPs etc.)!

@dfreeman
Copy link
Contributor

dfreeman commented Feb 6, 2023

The one open question I have (and I will defer to @dfreeman on) is: out of the box, should we wire up tsc or glint? I am broadly inclined to agree with one part of a comment from @NullVoxPopuli, that we “may as well set up Glint as well, since it's nearly 1.0”

I think I'm aligned there—we're pretty close to declaring 1.0 stable. I think there's one last design question we need to answer (or at least decide whether we can punt on answering), and beyond that there are a just couple more fixes/improvements we want to land at some point, but those shouldn't be blocking for a stable release.

@chriskrycho
Copy link
Contributor

Thanks for that confirmation, @dfreeman! Folks on this thread, I've opened emberjs/rfcs#902 to amend emberjs/rfcs#800 along the lines described here. Comments welcome there, and also feel free to implement along those lines!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants