Upgrades: TypeScript, Sync with Blueprint #424
Conversation
this renamed the file to cjs which made it necessary to have a separate commit after the rename to allow history to be correctly tracked on the file
version 2.7.0 introduced support for static and private fields required for this change
|
Why not 4.12 as minimum version? Any limitation? This would allow to have 2 previous majors with seemingly not much effort as not much (if any) changed in v5 |
|
I am not sure, 5.4 is just how this was handed to me -- I suspect we can:
it is rather annoying to configure ember try to have this wide range though. but I can copy paste from other addons |
| - embroider-safe | ||
| - embroider-optimized |
There was a problem hiding this comment.
should we be removing this?
There was a problem hiding this comment.
yes, because we always test embroider-optimized now. there is no point in testing less than optimized
| "targetFormat": "hbs", | ||
| "transforms": [] | ||
| }], | ||
| ["module:decorator-transforms", { "runtime": { "import": "decorator-transforms/runtime" } }], |
There was a problem hiding this comment.
should we use the runEarly: true flag?
There was a problem hiding this comment.
idk -- it's not in the blueprint -- what's it for?
| const { maybeEmbroider } = require('@embroider/test-setup'); | ||
| return maybeEmbroider(app, { | ||
| const { Webpack } = require('@embroider/webpack'); | ||
| return require('@embroider/compat').compatBuild(app, Webpack, { |
There was a problem hiding this comment.
this defeats the purpose of ember-try entirely; or am I missing something?
There was a problem hiding this comment.
nay -- we still test ember-source 3.28 all the way to canary
There was a problem hiding this comment.
but only on embroider, which is not a very realistic test imo
There was a problem hiding this comment.
We don't have anything that the difference in build system needs to care about, so why make our test setup more complex?
in an addon like this, the build system is for us, the maintainers, to better debug and maintain the addon, not for declaring support.
Also, we get to lean on the fact that embroider-strict is the most correct thing we can test against right now (unless we completely switch over to vite), so anything that works with embroider here is guaranteed to work with classic builds. It's the tests between embroider and ember-auto-import that guarantee that. We don't need to provide that coverage as well.
There was a problem hiding this comment.
if this was the builld system of the addon code itself I would agree but the purpose of this test-app is to simulate how the addon could be used in the wider ecosystem
you cannot say that every single app out there that might want to use tracked-buil-ins will be on fully static embroider which is what this change is saying
testing how classic build pipeline behaves with the output of the addon build step is the purpose of ember try - if I'm mistaken in any of the above statements please let me know, but otherwise I strongly believe this change is wrong
there is a reason why default v2 addon bp outputs a test app with maybeEmbroider and not fully static build code
There was a problem hiding this comment.
you cannot say that every single app out there that might want to use tracked-buil-ins will be on fully static embroider which is what this change is saying
naw, that's not what I'm saying -- I'm saying all embroider-strict compatible addons are compatible with less strict environments.
there is a reason why default v2 addon bp outputs a test app with maybeEmbroider and not fully static build code
Because it hasn't been updated in ages, and is still mostly the app blueprint from ember-cli.
There are at least 4 things I change about every v2 addon I make, and this is one of them.
Even at work, I put all the test-apps on embroider-strict, and they work perfectly in the non-embroider consuming apps.
But also, I haven't put much work in to wanting to update the blueprint myself, because i know we want to kill the test-app entirely, and have it be a vite environment in the same package as the addon itself -- removing the need for a monorepo entirely.
Now, vite does support things that embroider-webpack and classic don't support yet, so unless you know what to avoid, it is actually dangerous right now to only use vite (blindly).
Those features that embroider-vite has are:
- v2 addon w/ type=module support (this is broken in auto-import and embroider-webpack)
- I can't remember anything else rn, it might just be that
Co-authored-by: Alex <void-mAlex@users.noreply.github.com>
| @@ -55,6 +55,7 @@ function convertToInt(prop: number | string | symbol): number | null { | |||
| return num % 1 === 0 ? num : null; | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-declaration-merging | |||
There was a problem hiding this comment.
Hmm... what is this even merging with? Is this actually necessary?
There was a problem hiding this comment.
At the bottom of the file. It is needed ya. The way we make these things look like normal arrays is the declaration merge.
Object is doing the same thing
NullVoxPopuli
changed the title
SergeAstapov
changed the title
NullVoxPopuli
changed the title
SergeAstapov
changed the title
Continuing:
Changes:
PRs that can be extracted
PRs to do