-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add typecsript sample rollup config + exploration of different plugins' side-effects #1099
Add typecsript sample rollup config + exploration of different plugins' side-effects #1099
Conversation
@NullVoxPopuli if you could cover in some example, probably still worth having an example of shipping compiled templates. We have a monorepo with 70+ addons and having hbs pre-compiled may yield noticeable perf improvement (even some 30s on scale matters) |
absolutely! I kinda want to try this exact same thing at work (with similar scale, too!) the tldr is this: in your babel config, add: const { precompile } = require('@glimmer/compiler'); // hope this is the same version used by your copy of ember-source
const { resolve } = require;
module.exports = {
plugins: [
// other plugins
// the important bit for compiling away the templates
[
resolve('babel-plugin-ember-template-compilation'),
{
precompile,
enableLegacyModules: ['ember-cli-htmlbars'],
},
],
],
} |
I just learned about
Example ts config I'm using: https://github.com/NullVoxPopuli/ember-resources/blob/main/ember-resources/rollup.config.js#L40 |
I think using If so, I think that's likely to cause trouble, because we don't want apps to start resolving the unbuilt TS files and trying to compile them, because if the typescript compiler version doesn't match perfectly they are likely to get type errors from down inside the addon. Shipping the |
I tested this in my big work monorepo (not using ember-resources, but in another package in the monorepo) and without
and with
I tried this I don't think
100% agree. I added an additional type of test for my output files to help keep me honest as I change rollup configs. atm, my output is:
|
I tried to migrate I am basically using a similar setup like proposed here, using So the way Compilation outputimport { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';
import { _ as __decorate } from '../tslib.es6-4cf1a877.js';
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { inject } from '@ember/service';
import { assert } from '@ember/debug';
var TEMPLATE = hbs("<div\n {{did-insert this.register}}\n {{will-destroy this.unregister}}\n ...attributes\n>\n {{yield this.count}}\n</div>");
class PortalTargetComponent extends Component {
portalService;
get count() {
return this.portalService.getPortalCount(this.args.name);
}
register(element) {
assert('PortalTargets needs a name', this.args.name);
const options = {
multiple: this.args.multiple,
onChange: this.args.onChange
};
this.portalService.registerTarget(this.args.name, element, options);
}
unregister() {
this.portalService.unregisterTarget(this.args.name);
}
}
__decorate([inject('-portal')], PortalTargetComponent.prototype, "portalService", void 0);
__decorate([action], PortalTargetComponent.prototype, "register", null);
__decorate([action], PortalTargetComponent.prototype, "unregister", null);
setComponentTemplate(TEMPLATE, PortalTargetComponent);
export { PortalTargetComponent as default }; You can see there is still the Compilation outputimport { _ as _defineProperty } from '../defineProperty-1e8516b1.js';
import { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';
import { _ as __decorate } from '../tslib.es6-4cf1a877.js';
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { inject } from '@ember/service';
import { assert } from '@ember/debug';
var TEMPLATE = hbs("<div\n {{did-insert this.register}}\n {{will-destroy this.unregister}}\n ...attributes\n>\n {{yield this.count}}\n</div>");
class PortalTargetComponent extends Component {
constructor(...args) {
super(...args);
_defineProperty(this, "portalService", void 0);
}
get count() {
return this.portalService.getPortalCount(this.args.name);
}
register(element) {
assert('PortalTargets needs a name', this.args.name);
const options = {
multiple: this.args.multiple,
onChange: this.args.onChange
};
this.portalService.registerTarget(this.args.name, element, options);
}
unregister() {
this.portalService.unregisterTarget(this.args.name);
}
}
__decorate([inject('-portal')], PortalTargetComponent.prototype, "portalService", void 0);
__decorate([action], PortalTargetComponent.prototype, "register", null);
__decorate([action], PortalTargetComponent.prototype, "unregister", null);
setComponentTemplate(TEMPLATE, PortalTargetComponent);
export { PortalTargetComponent as default }; As you can see, we now have the class property removed in favor of defining it in the constructor, setting it effectively to And this exactly causes the problem! So basically the two step compilation of TS and then Babel is failing here AFAICT. An alternative would be reverting the approach to again only use the babel rollup plugin directly (with Any other ideas? |
BTW, @NullVoxPopuli I don't understand why you have these two Babel plugins, as Babel would not see any TypeScript or decorators code when running as part of |
@simonihmig oh, I've been there with https://github.com/ember-animation/ember-animated/blob/master/addon/babel.config.json TBH, I'm not sure we actually need to transpile away class properties at addon build time and I think app can do it based on targets |
Oh wow, that feedback was super fast, and more importantly it indeed fixed my issue! 🎉 Thanks a lot @SergeAstapov! 🙏 So for the record, the output is now like this: Compilation outputimport { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';
import { _ as __decorate } from '../tslib.es6-4cf1a877.js';
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { inject } from '@ember/service';
import { assert } from '@ember/debug';
var TEMPLATE = hbs("<div\n {{did-insert this.register}}\n {{will-destroy this.unregister}}\n ...attributes\n>\n {{yield this.count}}\n</div>");
class PortalTargetComponent extends Component {
get count() {
return this.portalService.getPortalCount(this.args.name);
}
register(element) {
assert('PortalTargets needs a name', this.args.name);
const options = {
multiple: this.args.multiple,
onChange: this.args.onChange
};
this.portalService.registerTarget(this.args.name, element, options);
}
unregister() {
this.portalService.unregisterTarget(this.args.name);
}
}
__decorate([inject('-portal')], PortalTargetComponent.prototype, "portalService", void 0);
__decorate([action], PortalTargetComponent.prototype, "register", null);
__decorate([action], PortalTargetComponent.prototype, "unregister", null);
setComponentTemplate(TEMPLATE, PortalTargetComponent);
export { PortalTargetComponent as default }; Which is almost the same, except for the removed class property! But this eliminates the problem I described above, that this gets transpiled further down the road in the app to assigning undefined in the constructor. This still leaves me a bit puzzled:
At least the PR is green now! 😍 |
Yeah, class props on its own are probably ok. But in my case it seems it was a combination of an (untranspiled) class property plus the transpiled decorator code (for that same property) that blew things up... |
I've been having issues with converting ember-headlessui |
it should be totally based on browsertargets., yeah agreed! Here is a bare minimum demo of
https://github.com/NullVoxPopuli/babel-transpilation-tests |
Progress on support for TS + components -- I have a working addon here: A couple things:
|
e63cf9f
to
40bd95f
Compare
I think this is now ready for re-review. with v3 of rollup-plugin-ts, we have babel doing all transpilation now, so things should be in a much better spot than before. I must note, that it seems in typescript > 4.3, declarationMaps are no longer generated -- but it seems a problem with TS-upstream, rather than anything here |
}, | ||
], | ||
[ | ||
resolve('@babel/plugin-proposal-decorators'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to specify the class-properties plugin.
By only specifying the decorators plugin, we leave properties "native" and untranspiled, since all browsers support class properties now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is fine, but your explanation for why it's fine is incorrect:
since all browsers support class properties now.
Browser support has nothing to do with it. That is a concern for apps, not addons.
The only reason we need to transpile the decorators here is that acorn doesn't parse them, so rollup blows up. As soon as that is fixed, we should drop decorator transpilation, and that won't depend on whether any browsers have implemented decorators.
const { resolve } = require; | ||
|
||
module.exports = { | ||
presets: [resolve('@babel/preset-typescript')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using resolve here, because I hate "plugin not found errors" in babel. Typos are much easier to catch earlier, and resolve helps out with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have both @babel/preset-typescript
and @babel/plugin-transform-typescript
? I think it should be one or the other. The only thing the preset does is install that plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin-transform-typescript is required when doing resolve
in the babel.config.js (by eslint + node:recommended)
if I just use strings for the plugin names, the issue is side-stepped.
happy to do whatever you think is best here
// These are the modules that should get reexported into the traditional | ||
// "app" tree. Things in here should also be in publicEntrypoints above, but | ||
// not everything in publicEntrypoints necessarily needs to go here. | ||
addon.appReexports([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an aside, is there anything we can do with appReexports
and publicEntrypoints
to either not need file extensions, or have some sort of assertions on over-specification of extensions? The plugins are very forgiving here, and I'm not sure what's "proper".
// It exists only to provide development niceties for you, like automatic | ||
// template colocation. | ||
// See `babel.config.json` for the actual Babel configuration! | ||
ts({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we also want to provide a sample tsconfig?
I've been using:
{
"compilerOptions": {
// Path resolution
"baseUrl": "./src",
"moduleResolution": "node",
// types are built by rollup-plugin-ts
"declarationDir": "./dist",
"emitDeclarationOnly": true,
"declaration": true,
// Build settings
"noEmitOnError": false,
"module": "ESNext",
"target": "ESNext",
// Features
"experimentalDecorators": true,
"allowJs": false,
"allowSyntheticDefaultImports": false,
// Strictness / Correctness
"strict": true,
"noImplicitAny": true,
"noImplicitThis": true,
"alwaysStrict": true,
"strictNullChecks": true,
"strictPropertyInitialization": true,
"noFallthroughCasesInSwitch": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"noImplicitReturns": true,
"paths": {
// No paths, no absolute imports, only node_modules imports allowed
// But fallback for type-overrides and such
"*": ["types/*"]
}
},
"include": [
"src/**/*",
"types/*"
]
}
Ideally, I would prefer if we can keep publicEntrypoints and appReexports arguments in terms of the publicly exposed filenames, which are always JS not TS. This is already how we treat components that are authored as standalone hbs files (in publicEntrypoints they appear as JS, not HBS, because the exact authoring format is an internal implementation detail). |
Thanks @NullVoxPopuli, did you confirm that the changes that we discussed and you implemented in 0b1408d actually work? I wasn't sure if a corresponding change is needed in the plugins. |
Yeah, I have a ts demo that I started long ago, and the output |
ok, so I gave up on https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo/ the rollup build fails when publicEntrypoints points only at JS (and all files on disk are TS). |
… publicEntrypoints is needed
Yes, so let's fix the publicEntrypoints and/or appReexports plugins so they accept the .js extensions, and then keep the .js extensions in these examples. |
Since we use the publicEntrypoints to match the file system, the only way to make this work would be to scan all files and assume that if a file matches without the specified extension in publicEntrypoints that that match (and all additional matching files?) are what we want to pull in to rollup -- brings up the question -- what happens when we encounter things rollup doesn't support? like, maybe This seems... more complicated? maybe confusing? |
This is consistent with our handling of component hbs files. It's not that we match anything, it's that we know a short list of extension mappings caused by the build. Right now we only predict the hbs -> js transformation, but we also have a list of extensions that normalize to js that we could also predict. |
PR for that here: #1223 |
// but we need the ember plugins converted first | ||
// (template compilation and co-location) | ||
transpiler: 'babel', | ||
browserslist: ['last 2 firefox versions', 'last 2 chrome versions'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have this? Browser support should be an app concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do need this -- the default is "something isn't close to what the browsers could natively support".
Without the browserslist,
- argument-destructuring is polyfilled
- nullish coalescing is polyfilled
- optional chaining is polyfilled
- etc
I agree that It absolutely should be an app concern to compile away these features, but I think addons should ship "as close to native" as they want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's definitely wrong to have this browserslist here. This implies that babel-preset-env is running. We need it not to run.
I think browserslist: false
may disable babel-preset-env entirely, just based on perusing the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the docs confirm it: https://github.com/wessberg/rollup-plugin-ts#babelpreset-env-behavior-and-how-to-opt-out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have been doing this also here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting browserslist to false added poplyfills for ownKeys and objectSpread in my addon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ef4 @NullVoxPopuli As discussed yesterday, I looked into this today. Our suspicion, that rollup-plugin-ts
does some unexpected magic, seems to be right unfortunately. I filed this issue: wessberg/rollup-plugin-ts#189
transpiler: 'babel', | ||
browserslist: ['last 2 firefox versions', 'last 2 chrome versions'], | ||
tsconfig: { | ||
fileName: 'tsconfig.json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we omit this, as this is just declaring what is already the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, I'll remove. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just kidding, it's required when hooks are present
// Allows us to use `exports` to define types per export | ||
// However, we can't use that feature until the minimum supported TS is 4.7+ | ||
declarationDir: './dist', | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this hook thing? Most of the options are already set in your tsconfig.json
example, except for declarationMap
, which we also could set there!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah -- kinda more for understandability than anything -- having the rollup config control all output is easier to reason about than having two places where output can be configured
rollup-plugin-ts
9f980ed
to
672470b
Compare
Gonna close this, because I think we've figured out everything we need to figure out w/r/t TS support in v2 addons (and there are demos of these in the wild). Also with the upcoming |
With
rollup-plugin-ts
https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo
@glimmer/component
is undefined"type": "module"
in the addon's package.json. We can't use"type": "module"
quite yetchore(internal): try new rollup config NullVoxPopuli/ember-statechart-component#244
Try new rollup config NullVoxPopuli/ember-resources#366
only utils, no app re-exports, compiled to single file
pending release of Addon Dev - Allow ts,gts,gjs files as publicEntrypoints #1106
pending fix for: Cannot import from
@glimmer/tracking/primitives/cache
#1020this approach convinced me to avoid the multi-input plugin
because it does this:
it has a bunch of weird imports.
I understand it importing my own stuff so that it doesn't get optimized away (but these are side-effecting imports?) very weird
Resolves
TODO
Figure out how to avoid specifying
input
at allFigure out why
setComponentTemplate
appears to be invoked twice per templatesample output from:
https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo
Potential solve:
Solve ts related double setComponentTemplate #1134
Ensure that app-js in package.json is updated
Ensure that template-only components can be imported
Ensure that at least test-support is its own file
Answer the questions
hbs
to opcode conversiontldr: the compiled format is tightly coupled to exact ember version, so it's unsafe to ship compiled templates.
it may make sense if you can guarantee that addons and apps will always have the same exact ember version
(maybe if you're doing a monorepo at work or something where everything is private)