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

I updated vite-plugin-solid and now namespaces merged with components are no longer allowed #145

Open
AFatNiBBa opened this issue Feb 5, 2024 · 11 comments

Comments

@AFatNiBBa
Copy link

AFatNiBBa commented Feb 5, 2024

Describe the bug

If I create a .tsx file with this content

function A() {
    return <>1</>
}

namespace A {
    export const a = 1;
}

console.log(A);

It throws this error when I load the page (Running vite dev)

Transform failed with 1 error:
ERROR: The symbol "A" has already been declared

I tried downgrading vite-plugin-solid and it worked

npm i vite-plugin-solid@2.8.0

(On the playground it always works)

Your Example Website or App

(localhost)

Steps to Reproduce the Bug or Issue

  1. Put a merged namespace declaration WITH A COMPONENT somewhere
  2. Run vite dev
  3. Go to the web page

Expected behavior

I expected this allowed TS syntax to not stop working

Screenshots or Videos

No response

Platform

  • OS: Windows
  • Browser: Edge
  • Version: 120.0.2210.77

Additional context

No response

@ryansolid ryansolid transferred this issue from solidjs/solid Feb 5, 2024
@lxsmnsyc
Copy link
Member

lxsmnsyc commented Feb 14, 2024

Okay so it seems that namespace A is actually A = {...} (an assignment, rather than a declaration).

It's a very weird edge case honestly, although I would've expected it to raise an error too if not for functions to be re-assignable, and if namespaces transformed into variable declarations. Not sure if this should be fixed.

Besides assignment to function A was there any other case where a namespace of similar name is desirable?

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Feb 14, 2024

update:

Upon checking, since we convert every function component into a HOC (declared via a variable), it seems that TS compiles weirdly for it.

Example:

const A = () => {

};

namespace A {
  export const hello = 'world';
}
const A = () => {
};
var A;
(function (A) {
    A.hello = 'world';
})(A || (A = {}));

and since we rely on HOC to apply HMR, this is most likely a wontfix. However, I would recommend just assigning to the function directly.

related:
microsoft/TypeScript#51417

@AFatNiBBa
Copy link
Author

AFatNiBBa commented Feb 19, 2024

Isn't it possible to use var?

var A = () => {
};
var A;
(function (A) {
    A.hello = 'world';
})(A || (A = {}));

@lxsmnsyc
Copy link
Member

@AFatNiBBa your example code shows why it won't work.

@AFatNiBBa
Copy link
Author

Do you transform the function to make HMR work BEFORE or AFTER transpiling typescript?
Because if you were to do it after you could declare the function with var

//     ↓
export var A = _$$component(_REGISTRY, "A", function A() {
    // ...
}, {
    location: "..."
});

And if you're doing it before the transpiling because if you were to do it after you would not know where the JSX was, then you could mark the function in some way (Maybe with a comment?) in order to still find it at the end

@lxsmnsyc
Copy link
Member

@AFatNiBBa

Do you transform the function to make HMR work BEFORE or AFTER transpiling typescript?

there's no TypeScript transpilation happening, as it is deferred to ESBuild.

And if you're doing it before the transpiling because if you were to do it after you would not know where the JSX was, then you could mark the function in some way (Maybe with a comment?) in order to still find it at the end

I'm not sure I understood the suggestion.

@AFatNiBBa
Copy link
Author

AFatNiBBa commented Feb 20, 2024

I'm not sure I understood the suggestion

Don't worry, if the transformation must happen at the TypeScript level, the suggestion doesn't apply anyway

@AFatNiBBa
Copy link
Author

Isn't it possible to change

export var A = _$$component(_REGISTRY, "A", function A() {
    // ...
}, {
    location: "..."
});

Into

import { createComponent } from "solid-js";

const RANDOM_NAME = _$$component(_REGISTRY, "A", function A() {
    // ...
}, {
    location: "..."
});

// Wrapper
export function A(x) {
    return createComponent(RANDOM_NAME, x);
}

// Now this is allowed
export namespace A {
    export const value = 56;
}

@lxsmnsyc
Copy link
Member

No, that would introduce some new layer of issues. I'm sorry, this might be a no-fix as TS is to be blamed here for this inconsistency. I could recommend you dropping the namespace usage (since it's not even recommended anymore by majority) but that's just one workaround.

@AFatNiBBa
Copy link
Author

TypeScript is not to blame this time in my opinion, because functions have always behaved differently from lambdas, It is the plugin's responsability to ensure that if I write a function then a function will come out instead of a lambda expression.

I think you shouldn't give priority to this issue, but it's something that has to be done eventually

@lxsmnsyc
Copy link
Member

I'm sorry but something about the current transform is never going to be touched, we haven't even solved the SSR problem with the current transform, we'll be digging a whole new level of complexity if we do this "component in a component" kind of thing (which btw, also ruins the dev experience at some point).

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

2 participants