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

Provide TypeScript compilation support when using ember-cli-typescript@4 or higher. #314

Merged
merged 27 commits into from
Feb 5, 2020

Conversation

jamescdavis
Copy link
Contributor

@jamescdavis jamescdavis commented Dec 19, 2019

This is based on a conversation between some of the ember-cli-typescript maintainers (including @dfreeman and @chriskrycho) and @rwjblue. The ember-cli-typescript side of this is happening in typed-ember/ember-cli-typescript#1018.

This moves management of @babel/plugin-transform-typescript to ember-cli-babel when ember-cli-typescript >= 4.0.0-alpha.1 is installed in the parent. It also adds @babel/plugin-proposal-optional-chaining and @babel/plugin-proposal-nullish-coalescing-operator when ember-cli-typescript >= 4.0.0-alpha.1 is installed because these are standard features in TypeScript >= 3.7.0 (and we can't add these conditionally because we can't be sure about the TypeScript version used in addons).

See typed-ember/ember-cli-typescript#1018 for the release plan.

@jamescdavis
Copy link
Contributor Author

The floating deps job has also failed recently for a docs-only PR. ¯\_(ツ)_/¯

@jamescdavis jamescdavis changed the title Add typescript transform plugin when e-c-ts >= 4.0.0-alpha Add TypeScript transform plugin when e-c-ts >= 4.0.0-alpha Dec 19, 2019
@rwjblue
Copy link
Member

rwjblue commented Dec 20, 2019

#315 should fix the floating deps tests.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I did a partial review with some inline comments, but didn't fully complete review. I'll try to poke at it a bit more this afternoon.

index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated
Comment on lines 330 to 332
if (typeof emberCLIBabelConfig.enableTypeScriptTransforms === 'boolean') {
return emberCLIBabelConfig.enableTypeScriptTransforms;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is great, it allows for a really nice escape hatch when our autodetection may fail (which AFAICT shouldn't happen, but it is still nice to be prepared).

Can you add this to the documentation in the README?

Copy link
Contributor Author

@jamescdavis jamescdavis Jan 8, 2020

Choose a reason for hiding this comment

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

There are also people wishing to use TypeScript without integrated type-checking, which this option would enable (by not having to install ember-cli-typescript just to get TypeScript transpilation).

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 also documented the automatic behavior and incompatibility of this option with e-c-ts < 4.0.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated
if (this.parent === this.project) {
this.project.ui.writeWarnLine(`${
this._parentName()
} has added the optional chaining plugin to its build, but ember-cli-babel provides this by default now when ember-cli-typescript >= 4.0 and typescript >= 3.7 are installed! You can remove the transform, or the addon that provided it.`);
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check, this is added by default because TypeScript requires it? Also, how do you know that typescript >= 3.7 is being used (does e-c-ts bring a minimum of [email protected]?)?

Copy link
Member

Choose a reason for hiding this comment

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

Side note, we may want to make a more official policy to include things like this in the future (e.g. "stage-3 proposals are added by default"), not sure but that would probably require an RFC.

What do you think @pzuraq?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because these features are part of TS >= 3.7. The reason these have to be added unconditionally is because we don't know the TypeScript version an addon might have used.

@jamescdavis jamescdavis force-pushed the add_typescript_transform_plugin branch from ff592d3 to 5fb2aa0 Compare January 8, 2020 12:46
@dfreeman
Copy link
Contributor

dfreeman commented Jan 8, 2020

@rwjblue one thing the typed-ember folks had talked about as part of our v4 release was explicitly excluding .d.ts files from emit. Currently, any of those that you might have in your app/addon tree will each produce an empty module definition with the trailing .d (define('foo/bar/baz.d', ...)) in the final output, which is a waste of bytes.

Do you have thoughts on how we might best accomplish that?

@rwjblue
Copy link
Member

rwjblue commented Jan 9, 2020

@dfreeman - When running our toTree we can add a new Funnel(rawInput, { exclude: ['**/*.d.ts'] }) before actually passing to compilation with broccoli-babel-transpiler.

@rwjblue
Copy link
Member

rwjblue commented Jan 9, 2020

Tweaking this

https://github.com/babel/ember-cli-babel/blob/442ec64a737a8cd638ca2fa064643bf258aa8d71/index.js#L44-L58

To be something like:

  transpileTree(inputTree, config) {
    let description = `000${++count}`.slice(-3);
    let postDebugTree = this._debugTree(inputTree, `${description}:input`);

    let options = this.buildBabelOptions(config);
    let output;
    if (this._shouldDoNothing(options)) {
      output = postDebugTree;
    } else {
      let BabelTranspiler = require('broccoli-babel-transpiler');
      
      let transpilationInput = postDebugTree;

      if (this._shouldCompileTypescriptOrWhateverTheMethodIsCalled()) {
        transpilationInput = this._debugTree(new Funnel(postDebugTree, { excludes: ['**/*.d.ts']}), `${description}:filtered-input`);
      }
      output = new BabelTranspiler(transpilationInput, options);
    }

    return this._debugTree(output, `${description}:output`);
  },

@dfreeman
Copy link
Contributor

dfreeman commented Jan 9, 2020

👍 Thanks, that makes sense! I wasn't sure if there was a cleverer way than just funneling the input, but I guess that's exactly what we'd be doing if we implemented outside of ember-cli-babel anyway

@jamescdavis
Copy link
Contributor Author

@rwjblue @dfreeman When I saw this, I was hoping there was a way to filter out .d.ts files with a negative lookbehind in the extensions list (set extensions to ['js', '(?<!\\.d\\.)ts']) . But alas, that RegExp is only used to pull module names out of file names (not to filter extensions) 😞. I think it would require modifying both ember-cli-preprocess-registry and broccoli-persistent-filter to handle specifying extensions as regexes.

@jamescdavis jamescdavis changed the title Add TypeScript transform plugin when e-c-ts >= 4.0.0-alpha Add TypeScript transform plugin when e-c-ts >= 4.0.0-alpha.1 Jan 18, 2020
@jamescdavis
Copy link
Contributor Author

[email protected] has been released, so this is now ready to be included in an ember-cli-babel release when we are satisfied with it. Once released, we will update ember-cli-typescript to depend on that version of ember-cli-babel (and likely release as 4.0.0-beta.1 or 4.0.0-rc.1 for people to test before we cut a stable release). IMHO, we don't need a pre-release of ember-cli-babel for this because the changes will only take effect when combined with a pre-release of ember-cli-typescript (moving to a stable release once we're confident we didn't miss something).

@jamescdavis jamescdavis marked this pull request as ready for review January 18, 2020 15:48
@jamescdavis jamescdavis force-pushed the add_typescript_transform_plugin branch 2 times, most recently from 0a85eee to 45e006f Compare January 27, 2020 21:46
As of 7.8.0, @babel/preset-env enables optional-chaining and
nullish-coalescing by default.
@jamescdavis jamescdavis force-pushed the add_typescript_transform_plugin branch from 45e006f to 4ab55d1 Compare January 27, 2020 21:48
@jamescdavis
Copy link
Contributor Author

Since @babel/preset-env now enables optional chaining and nullish-coalescing by default (as of 7.8.0), I've updated this to not add them manually. I've also renamed the method to add the plugin and the flag to manually enable the TypeScript transform to be singular since they now only affect one plugin.

@rwjblue rwjblue changed the title Add TypeScript transform plugin when e-c-ts >= 4.0.0-alpha.1 Provide TypeScript compilation support when using ember-cli-typescript@4 or higher. Feb 5, 2020
@rwjblue rwjblue merged commit 2b697ca into emberjs:master Feb 5, 2020
@rwjblue rwjblue mentioned this pull request May 29, 2020
@jamescdavis jamescdavis deleted the add_typescript_transform_plugin branch January 21, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants