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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

bindable decorator should not be giving a runtime error #1980

Closed
dkent600 opened this issue May 22, 2024 · 28 comments
Closed

bindable decorator should not be giving a runtime error #1980

dkent600 opened this issue May 22, 2024 · 28 comments

Comments

@dkent600
Copy link

馃悰 Bug Report

bindable decorator is giving a runtime error.

馃 Expected Behavior

should not give the error

馃槸 Current Behavior

bindable decorator is giving runtime error:

Uncaught TypeError TypeError: Cannot read properties of undefined (reading 'au:annotation:bindables')
at decorator (c:\Users\dkent\Documents\Projects\douglaskent\node_modules@aurelia\runtime-html\src\bindable.ts:102:40)
at bindable (c:\Users\dkent\Documents\Projects\douglaskent\node_modules@aurelia\runtime-html\src\bindable.ts:110:5)

It is happening on this line:

const metadata = (context.metadata[baseName] ??= createLookup()) as Record<string, BindableDefinition>;

where context equals the string "inline" and context.metadata is undefined.

baseline equals 'au:annotation:bindables'

馃拋 Possible Solution

馃敠 Context

I'm stuck. The app won't run. You may reproduce as follows:

  1. clone the repo and look at this commit: dkent600/douglaskent@1a5de33
  2. run npm install
  3. run 'npm run start'

You will see the error message in the browser console.

馃捇 Code Sample

I am using this repo at this commit: dkent600/douglaskent@1a5de33

馃實 Your Environment

Software Version(s)
Aurelia v2.0.0-beta.17
Language Typescript, v5.4.2
Browser Edge
Bundler vite and rollup
Operating System Windows 11
NPM/Node/Yarn NPM 10.5.0

Definitely love Aurelia, for a long time

@Sayan751 Sayan751 assigned Sayan751 and unassigned Sayan751 May 22, 2024
@Sayan751
Copy link
Contributor

Hi @dkent600, it seems like a vite+esbuild issue. As the latest esbuild added support for the native decorators, it is worth updating the version of esbuild. Moreover, as the parameter decorators are not yet supported by the new decorator specs, those instances needs to be refactored. The Au2 release notes mention this.

I have tried the aforementioned changes in your repo. Here is a change.patch that you can apply and check.

However, it seems that the decorators are still not transpiled. Unfortunately, I am not much familiar with vite and/or esbuild. @3cp @bigopon Do you guys notice anything configuration change that needs to be made in order to transpile the code view vite/esbuild?

@bigopon
Copy link
Member

bigopon commented May 22, 2024

It seems the version of vite in the repo is quite old, maybe let's test it with the latest first @dkent600 ? it's 5.2.11

@dkent600
Copy link
Author

I have tried the aforementioned changes in your repo. Here is a change.patch that you can apply and check.

Thanks @Sayan751! FYI I went ahead and committed those changes to master, here: dkent600/douglaskent@fb3c171

@dkent600
Copy link
Author

It seems the version of vite in the repo is quite old, maybe let's test it with the latest first @dkent600 ? it's 5.2.11

@bigopon The version that @Sayan751 is testing with is 5.2.11 which is only three weeks old.

@dkent600
Copy link
Author

dkent600 commented May 22, 2024

@Sayan751 @bigopon FYI I updated the vite config to use @aurelia/vite-plugin and made changes to the vite.config. The code is compiling again, but the vite build of the app is having this problem at runtime when it hits @customElement({ name: "resume", template }):

Uncaught SyntaxError SyntaxError: Invalid or unexpected token
at (program) (c:\Users\dkent\Documents\Projects\douglaskent\src\pages\resume\resume.ts:18:1)

The changes are here: dkent600/douglaskent@e1c46f9

PS: There is also a production build issue from rollup. I will investigate.

@dkent600
Copy link
Author

dkent600 commented May 23, 2024

Uncaught SyntaxError SyntaxError: Invalid or unexpected token
at (program) (c:\Users\dkent\Documents\Projects\douglaskent\src\pages\resume\resume.ts:18:1)

Both the above error and the production build issue from rollup go away if I restore the following to tsconfig.json:

    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,

Of course that causes other issues to ensue, namely in @customElement:

TypeError: Cannot read properties of undefined (reading 'addInitializer')

@bigopon
Copy link
Member

bigopon commented May 23, 2024

I'm not sure what's causing the prod issue, since everything looks fine.

"experimentalDecorators": true,
"emitDecoratorMetadata": true,

We should not need these 2, since it's for the old decorators code, we no longer have it.

@Sayan751
Copy link
Contributor

It seems like a configuration issue to me, since the latest version of esbuild can transpile the decorators. Maybe something in the in vite config or rollup is missing.

@3cp
Copy link
Member

3cp commented May 23, 2024

@bigopon a side issue. Our plugin-conventions is pinned to one typescript version, should we relax it to "^..."?.

"node_modules/@aurelia/plugin-conventions": {
      "version": "2.0.0-beta.17",
      "resolved": "https://registry.npmjs.org/@aurelia/plugin-conventions/-/plugin-conventions-2.0.0-beta.17.tgz",
      "integrity": "sha512-m14ML6Atfl5B389e1Hxj27H/f1lTm33HyIvgdLvUp+jLUaQnTlf5bqiSmNQLHlrhcnskH+nKfG6ngJ3umEPBgw==",
      "dev": true,
      "dependencies": {
        "@aurelia/kernel": "2.0.0-beta.17",
        "@aurelia/metadata": "2.0.0-beta.17",
        "@aurelia/platform": "2.0.0-beta.17",
        "@aurelia/runtime": "2.0.0-beta.17",
        "@aurelia/runtime-html": "2.0.0-beta.17",
        "modify-code": "^2.1.3",
        "parse5": "^5.1.1",
        "typescript": "5.4.2"
      },
      "engines": {
        "node": ">=14.17.0"
      }
    },

@3cp
Copy link
Member

3cp commented May 23, 2024

@dkent600 you patched our html import to .html?raw, I don't think that would ever work with au2.

Our plugin already rewrote html file into js code, it's not raw text anymore.

My bad, missed your condition.

@bigopon
Copy link
Member

bigopon commented May 23, 2024

relax it to "^..."?.

Yes we should, completely missed it, thanks @3cp

@dkent600 I tried to debug by removing everything and only had these:

  • main.ts

    @dec
    class App {}
    
    function dec() {
      console.log('???');
    }
    
    document.body.textContent = String(new App());
  • vite.config.ts

    export default defineConfig({
      server: {
        port: 9000,
        strictPort: true,
      },
    })

And it still couldn't compile the decorator! Will check further.

@dkent600
Copy link
Author

For what it is worth, running npx tsc has no problem, with tsconfig changes:

    //"noEmit": true,
    "outDir": "dist/tsc",
    // "experimentalDecorators": true,
    // "emitDecoratorMetadata": true,

@bigopon
Copy link
Member

bigopon commented May 23, 2024

I quite give up on this ... but the good news is if you use true for enableConventions in the viteconfig, you'll not have to worry about the decorators.
(it doesn't compile with even simplest app & config whatsoever)

@dkent600
Copy link
Author

dkent600 commented May 23, 2024

I quite give up on this ... but the good news is if you use true for enableConventions in the viteconfig, you'll not have to worry about the decorators. (it doesn't compile with even simplest app & config whatsoever)

@bigopon I have set enableConventions to true in vite.config but the vite compile still fails on the decorator with:

Uncaught SyntaxError SyntaxError: Invalid or unexpected token
at (program) (c:\Users\dkent\Documents\Projects\douglaskent\src\pages\resume\resume.ts:18:1)

This can be seen in this commit: dkent600/douglaskent@fdd152c

@3cp
Copy link
Member

3cp commented May 23, 2024

I tried too. I think the setup somehow skipped typescript compilation which Vite should do automatically for .ts files. So even conventions handled the decorator, user code still fails with decorator.

I could not figure out what happened.

@bigopon
Copy link
Member

bigopon commented May 24, 2024

Not that this helps, but I'll just post anyway. I saw that the presence of

@esbuild-plugins/node-globals-polyfill

force npm to install esbuild wasm, removing it causes npm to take normal esbuild package.
image

Btw, when I change to main.js, and the index.html to:

<!DOCTYPE html>
<html>
  <head>
    <script type="module" src="src/main.js"></script>
  </head>
  <body>
  </body>
</html>

it still doesn't build ... well...!

@dkent600
Copy link
Author

dkent600 commented May 24, 2024

I quite give up on this ... but the good news is if you use true for enableConventions in the viteconfig, you'll not have to worry about the decorators. (it doesn't compile with even simplest app & config whatsoever)

@bigopon Assuming this issue is addressed by Vite, which do you generally recommend for `enableConventions'? true or false? I am not clear what is its purpose.

@dkent600
Copy link
Author

dkent600 commented May 24, 2024

I created a branch for this issue:

https://github.com/dkent600/douglaskent/tree/decorators-issue

And a ticket for vite: vitejs/vite#17308 (I hope I didn't make any mistakes with the facts of the matter)

@dkent600
Copy link
Author

I have a feeling that you guys and Vite may not be aligned about what should be happening here. In the given scenario, should the decorator syntax be getting transpiled for the browser, and if so, to what?

Please see: vitejs/vite#17308 (comment)

@3cp
Copy link
Member

3cp commented May 27, 2024

Sounds like

    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,

is the temporary solution here before esbuild follows tsc behaviour.

@dkent600
Copy link
Author

dkent600 commented May 28, 2024

Sounds like

    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,

is the temporary solution here before esbuild follows tsc behaviour.

@3cp I tried that before (see above) but it results in the runtime error:

TypeError: Cannot read properties of undefined (reading 'addInitializer')

@3cp
Copy link
Member

3cp commented May 28, 2024

Sounds like

    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,

is the temporary solution here before esbuild follows tsc behaviour.

@3cp I tried that before (see above) but it results in the runtime error:

TypeError: Cannot read properties of undefined (reading 'addInitializer')

Oh yes. Probably esbuild's decorator helper (in the compiled code) is for legacy decorator spec (honours experimentalDecorators), while Aurelia's decorator is using latest spec.

@dkent600
Copy link
Author

Status: Awaiting the fix in vite to address this issue that is apparently due to vite not using the latest version of esbuild.

@bigopon
Copy link
Member

bigopon commented May 29, 2024

Thanks @dkent600 , I'm not sure what vite does internally, because even with overrides in package json to ensure esbuild version 0.21.3, it still didn't compile, as shown in my comment above.

@dkent600
Copy link
Author

Thanks @dkent600 , I'm not sure what vite does internally, because even with overrides in package json to ensure esbuild version 0.21.3, it still didn't compile, as shown in my comment above.

Agreed @bigopon , I had the same thought. But they are claiming it fixes the issue. Will see...

@3cp
Copy link
Member

3cp commented May 29, 2024

This setting does fix the issue. I will put it in our WIP vite skeleton.

export default defineConfig({
  esbuild: {
    target: 'es2022',
  },

@dkent600
Copy link
Author

dkent600 commented Jun 1, 2024

Confirmed this setting does fix the issue: #1980 (comment)

If there has yet been a Vite release of what was supposed to be a fix, it is not sufficient.

@dkent600
Copy link
Author

dkent600 commented Jun 1, 2024

Fixed by #1980 (comment)

Thanks for everyone's help!

@dkent600 dkent600 closed this as completed Jun 1, 2024
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

No branches or pull requests

4 participants