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 built-in blueprints to TypeScript #19962

Merged
merged 14 commits into from
Mar 14, 2022

Conversation

cafreeman
Copy link
Contributor

@cafreeman cafreeman commented Feb 10, 2022

This PR is the ember.js counterpart to the work done in ember-cli to add support for typescript blueprints, both of which constitute the implementation of emberjs/rfcs#776.

This PR converts many (though not all) of the built-in ember blueprints. Notably, it excludes the mixin blueprint as well as the older test blueprints for both mocha and qunit. The only test blueprints that were converted were the post-testing overhaul blueprints (i.e. the ones that are now the defaults).

In order to prevent any compatibility issues that could arise from someone upgrading ember-source while still on an older version of Ember CLI, this PR also includes a polyfill for the typescript blueprint functionality.

@cafreeman cafreeman force-pushed the cfreeman/typescript-blueprints branch from 217253c to 6afcd72 Compare February 10, 2022 21:21
@@ -64,6 +66,7 @@
"ember-cli-normalize-entity-name": "^1.0.0",
"ember-cli-path-utils": "^1.0.0",
"ember-cli-string-utils": "^1.1.0",
"ember-cli-typescript-blueprint-polyfill": "^0.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update this to ^1.0.0 once we know there aren't any other changes needed.

@@ -0,0 +1,6 @@
export function initialize(appInstance) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we call this "owner" instead? In practice, that is what it is...

Suggested change
export function initialize(appInstance) {
export function initialize(owner) {

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2022

This looks great!!! I think the only thing left is to feature flag this so that we can easily toggle it on/off without reverting. Unfortunately, we don't have infrastructure setup to be able to read the @ember/canary-features module in node-land so it's a tad more annoying/difficult that more standard runtime feature flagging.

What I think we should do is:

  • Create a shared utility at blueprints/-can-emit-typescript.js, that looks something like:
// TODO: this should be reading from the @ember/canary-features module
// need to refactor broccoli/features.js to be able to work more similarly
// to https://github.com/emberjs/data/pull/6231
const EMBER_TYPESCRIPT_BLUEPRINTS = false;

module.exports = function() {
  return 'EMBER_TYPESCRIPT_BLUEPRINTS' in process.env 
    ? process.env.EMBER_TYPESCRIPT_BLUEPRINTS
    : EMBER_TYPESCRIPT_BLUEPRINTS;
};
  • Then update the polyfill to accept a second parameter (either options or a boolean) that allows us to force downlevel everyone to .js
  • Then update the blueprints here to test with that environment variable on and off (I think the output is actually nearly identical, with the only difference being the extension?)

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2022

Chatted with @cafreeman offline, and I think he's come up with a better (and simpler) way to do the feature flagging.

@cafreeman cafreeman force-pushed the cfreeman/typescript-blueprints branch from 542626c to ef25864 Compare February 16, 2022 19:01
@rwjblue
Copy link
Member

rwjblue commented Mar 1, 2022

Eeeck. Looks like we have some conflicts now, my apologies RE: the delays landing this.

@cafreeman - Mind rebasing / fixing things up?

@cafreeman cafreeman force-pushed the cfreeman/typescript-blueprints branch from ef25864 to f9661d7 Compare March 14, 2022 18:25
@cafreeman
Copy link
Contributor Author

@rwjblue Just resolved all the conflicts, we should be good to go now!

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