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

feat(solid): initial draft implementation #1096

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

raveclassic
Copy link

@raveclassic raveclassic commented Oct 23, 2022

Motivation

This PR introduces an initial implementation of linaria processor for solid.

Summary

The idea is to completely eliminate any extra runtime components and transpile the code into intermediate representation suitable for further transpilation with solid (this means, the processor doesn't transpile the code down to solid's output, it should be the responsibility of the end developer to pipe transformations correctly: first goes @linaria/solid, then the code must be transformed by solid with either babel plugin, vite plugin etc.)

Test plan

The tests are not working at the moment as it turns out it's quite hard to write any meaningful tests without the actual transpilation step. Testing snapshots seems too fragile and unmaintainable.

Instead I tried to take an approach on transpiling the code of tests with linaria+solid in a custom jest transform. It works but I'd still like to avoid testing stringified (or jsonified) output. Instead, it would be really cool to make use of JSDom to run the output, render generated components and test interaction with them. E.g. a reactive prop on a component must update components runtime structure etc.

At the moment I'm able to transpile the code with both jest transform (seamlessly transpiles the whole test and we can use regular TSX to write the code - huge win) and with a custom transpile step (takes a stringified input and transpiles down to solid's output - not so good as we lose TSX and still need to eval the code somehow).

Another thing to consider is that jest transform throws away generated css and I'd like to render it with JSDom so that interactions with components can be fully tested (e.g. when we set a prop on a component, component's color turns blue).
But this would require adding a build step with esbuild/vite/etc and somehow integrate it into jest+jsdom - the first thing that comes to mind is a custom jest environment based on jest-environment-jsdom, this might work. Also we could take a look at @testing-library/react and see what they are doing and try to implement the same things in the jest environment.

All these turned out to be much more work, so I stopped on that as unfortunately I don't have enough time at the moment.

Still, I'm opening the PR to demonstrate possible implementation of processor, as @Anber suggested in DM 👌

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2022

⚠️ No Changeset found

Latest commit: b03f733

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@usmanyunusov
Copy link

Снимок экрана 2022-10-23 в 14 45 18

Our implementation

>;
};

export declare const styled: Styled & StyledJSXIntrinsics;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an oversimplified version of styled. Why we can't use the original one from @linaria/react?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember exactly, but there were some rare cases where typings just didn't work (props just didn't type check). I'll try to find that case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a simple comparison of the old signature and the new one (simplified).
On the left is original, on the right - new.
image

  • T1: foo prop is allowed in original whereas it's not allowed in the new because it's not declared;
  • T2: bar is now not allowed in original, maybe because the component now has props; it's still not allowed in new because it's not declared;
  • T3: mostly the same as T1 - foo is not declared but is allowed in original; note - bar is allowed in new because type checker fails fast on the foo prop, it still rejects bar if foo is removed because none of them are declared
  • T4: this case is not supported at all in original
  • T5: this case is not supported at all in original
  • T6: this case is not supported at all in original

@Anber how about shipping simplified typings first and see how it goes/gather some feedback later?

packages/solid/src/index.ts Outdated Show resolved Hide resolved
packages/solid/package.json Outdated Show resolved Hide resolved
@usmanyunusov
Copy link

Work in progress on this thread?

@rockwotj rockwotj mentioned this pull request Nov 17, 2022
@raveclassic
Copy link
Author

@usmanyunusov yeah, in progress but I have very little time now to work on it.

@raveclassic
Copy link
Author

raveclassic commented Nov 25, 2022

@Anber smth weird is happening to test snapshots in interop package, I have newlines removed in my output but they are expected in snapshots. This is weird since the repo is using pnpm and there should be no package version collisions.
image

What's interesting is that it's fine on master branch 🤔

@raveclassic
Copy link
Author

I don't know why, but adding "@babel/core": "7.18.9" (which is the same exact version as in the repo root) to devDependencies in /packages/solid/package.json updates babel's transitive dependencies 🤷‍♂️
image

@raveclassic
Copy link
Author

@Anber pnpm/pnpm#4868

@raveclassic
Copy link
Author

ok, I finished the basic example built with solid-js, vite, @linaria/vite, and vite-plugin-solid, it's available at /examples/solid, the command to run is the regular pnpm start

@raveclassic raveclassic marked this pull request as ready for review November 25, 2022 18:10
@raveclassic raveclassic requested a review from Anber November 25, 2022 19:44
@raveclassic
Copy link
Author

@Anber ready for review

@raveclassic
Copy link
Author

raveclassic commented Nov 26, 2022

@Anber thanks for enabling the CI on this PR 🙏
there's something weird, I'm also on Node v16.16.0, but CI says it can't install node typings https://github.com/callstack/linaria/actions/runs/3554400920/jobs/5970646566#step:7:369
But they are there, and locally everything is fine 🤷‍♂️

No, they are missing, adding them in a moment

@usmanyunusov
Copy link

🙏

@Tetrax-10
Copy link

when can i expect this feature release?

@shiro
Copy link

shiro commented Dec 21, 2022

I just tested it quickly and don't fully understand solid, please correct me if I'm wrong.
This seems to lose reactivity on props in some cases:

const HelloWorldSolid = () => {
    const [count, setCount] = createSignal(0);

    return (
        <div>
            <Heading style={{"margin-top": `${count()}px`}}>
                count: {count()}
            </Heading>
            <button onClick={() => setCount((c) => c + 1)}> increment</button>
        </div>
    );
};
const Heading = styled.h1`
  background: red;
`;

The text in the heading updates correctly, but the margin is not applied.
If you replace Heading with h1 it works.

edit:
By quickly checking the transform, it looks like getStyleConstantValueExpression does unwrap props.style which leads to loss of reactivity. Perhaps wrapping the declaration in createMemo would propagate reactivity, but there might be more performant ways to handle it.

@NightmanCZ90
Copy link

Is this PR abandoned?

@ivancuric
Copy link

What's up with this PR?

@MrFoxPro
Copy link
Contributor

MrFoxPro commented Mar 12, 2024

just use tw/unocss + attributify
much better dx: framework/language agnostic, shorter then inlining css, no need to write style merging logic for every component, just spread props.

@ivancuric
Copy link

ivancuric commented Mar 13, 2024

just use tw/unocss + attributify much better dx: framework/language agnostic, shorter then inlining css, no need to write style merging logic for every component, just spread props.

This is just another DSL. I am looking for a way to write actual CSS, not tailwind shorthands in made up attributes that may or may not clash with actual JSX props or HTML attributes

@MrFoxPro
Copy link
Contributor

MrFoxPro commented Mar 13, 2024

just use tw/unocss + attributify much better dx: framework/language agnostic, shorter then inlining css, no need to write style merging logic for every component, just spread props.

This is just another DSL. I am looking for a way to write actual CSS, not tailwind shorthands in made up attributes that may or may not clash with actual JSX props or HTML attributes

There are some drawbacks, but I think it's much better then using linaria. You don't need this rocket-science euristic compilier that slows down dev builds and potentially breaks your code.

You can start with simple pure class="". Or you can even try web components, so you will also co-locate your css with components. These are much better options for any projects in long-term run, because that tools are much easier to work with and maintain, also they're language-agnostic.
Linaria, on the other hand, has strong legacy vibes.

I spent year with unocss after linaria and it feels so much better to work with.

@raveclassic
Copy link
Author

raveclassic commented Mar 13, 2024

@MrFoxPro let's keep this PR focused on linaria and solid. This is not the right place to promote failwind or any other framework.

@raveclassic
Copy link
Author

@Tetrax-10 @NightmanCZ90 @ivancuric I see all your comments here but unfortunately I don't have spare time to work on this. Your are kindly welcome to take on the initiative here 🙏

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

Successfully merging this pull request may close these issues.

8 participants