WIP trying to turn a build error into a runtime error

[mansona]

Hey @ef4 👋

This is a very crude first take on the thing that we discussed yesterday. I'm opening this PR before even checking any tests or anything because I wouldn't mind a bit of help with the TS implementation.

I feel like I have made a decent attempt to make TS happy but I am just not familiar with the definition syntax in ensureLinked on webpack.ts 🤔

private ensureLinked({
    packageName,
    packageRoot,
  }: {
    packageName: string;
    packageRoot: string;
  }): void

it's also a bit hard to google so if you could help me a little with this that would be much appreciated 🎉

[ef4]

This is the combination of two different features, which are slightly tricky to read because they look very similar but mean different things.

The first feature is plain javascript object destructuring:

ensureLinked({ packageName, packageRoot }) {
}

which is shorthand for

ensureLinked(params) {
  let packageName = params.packageName;
  let packageRoot = params.packageRoot;
}

The second feature is adding a type declaration for the above params argument:

ensureLinked(params: { packageName: string; packageRoot: string }) {
}

So to summarize, this is declaration function that accepts an object, and the object should have packageName and packageRoot with properties that are strings.

A way to refactor this to be more readable would be to declare a standalone type (but this has the downside that you have to go look elsewhere to see the type definition):

interface ThingToBeLinked {
  packageName: string; 
  packageRoot: string
}

class {
  ensureLinked({ packageName, packageRoot }: ThingToBeLinked) {
  }
}

[mansona]

Perfect! thanks for that 🎉 your incredibly detailed explanation really helped me to break apart what was happening and apply the same technique that I've been using elsewhere 👍

I've applied the basic version of what you and I discussed now and I think I have the types correct (or close to correct, maybe with some naive assumptions dotted throughout).

I'm running the test suite now locally to see if anything pops out, but I suspect that we might need to change any test that looked for that build error to instead boot the app and see the runtime error 🤔

[ef4]

There are end-to-end tests here that could exercise the runtime error:

https://github.com/ef4/ember-auto-import/blob/main/test-scenarios/dynamic-import-test.ts

Also the splitter has tests here that can unit test the splitter output:

https://github.com/ef4/ember-auto-import/blob/main/packages/ember-auto-import/ts/tests/splitter-test.ts

On Sat, Jul 2, 2022 at 9:30 AM Chris Manson @.***> wrote:

Perfect! thanks for that 🎉 your incredibly detailed explanation really helped me to break apart what was happening and apply the same technique that I've been using elsewhere 👍

I've applied the basic version of what you and I discussed now and I think I have the types correct (or close to correct, maybe with some naive assumptions dotted throughout).

I'm running the test suite now locally to see if anything pops out, but I suspect that we might need to change any test that looked for that build error to instead boot the app and see the runtime error 🤔

— Reply to this email directly, view it on GitHub https://github.com/ef4/ember-auto-import/pull/528#issuecomment-1172898755, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACN6MRVIHGN37LLI3L7AFTVSA76TANCNFSM52NIKA4A . You are receiving this because you were mentioned.Message ID: @.***>

[mansona]

Thanks @ef4, I've been able to use those 2 tests to verify that things were doing what I expected. I have now been able to get the template in webpack.ts to do the right thing, and I've verified that this line https://github.com/ef4/ember-auto-import/pull/528/files#diff-8b109a24d021f0efea4c42a6a90ea5afa9f93ff20f7d6528cea5d7f4ddd35a60R60 needs to stay inside the defined module (i.e. inside the d() call) or else it would be a runtime error but any time the code was loaded and not dependant on if the code path was executed.

The issue is that I've not been able to get this to fail the way I want it to. I have focused on the dynamic import right now since the dynamic template import can be a bit more tricky, but essentially the tests aren't failing with the error that we're defining even though I've verified locally (in a very clunky way) that the error is being exported into the build output correctly.

I don't know if I'm missing something subtle here but I was wondering if you can take a look and unblock me?

[mansona]

@ef4 so I'm getting much closer with this implementation (thanks to a pairing session with @nickschot last week 🎉 )

I would love a bit of guidance now if you have the time, even just approving the workflow and seeing the error message would be useful at this stage. I tried running this locally with an extra test-app in the packages and I got that test to "fail correctly" but now the error message seems to just have the route name in it 🤔 which is a bit odd.

I have been a bit confused with the types of imports that should fail and how to get them to go through the correct pathways. For example, I tried to add a route that did something like this:

return importSync(`/some-other-target.js`);

expecting that to end up in the Webpack generated file but it doesn't. Then I tried:

return import('/thing');

which does end up in the Webpack generated file but actually in the browser it just tries to do a real dynamic import 🙃 I will try to make it to the next office hours to see if we can move this forward but it can be a bit hard for me and timezones etc. so it would be better if we could progress async for now 👍

[mansona]

Closing in favour of https://github.com/ef4/ember-auto-import/pull/530 👍