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

dhow rewrite #12

Open
fs-c opened this issue Mar 14, 2021 · 5 comments · May be fixed by #13
Open

dhow rewrite #12

fs-c opened this issue Mar 14, 2021 · 5 comments · May be fixed by #13

Comments

@fs-c
Copy link
Contributor

fs-c commented Mar 14, 2021

While working on improving tests/testability and supporting _app.js, I accidentally rewrote dhow in the last couple of days, . You can take a look at the changes in the rewrite branch of my fork.

Some changes:

  • min-document and associated logic is replaced with a custom (very basic) virtual DOM implementation consisting exclusively of a VNode class that can be stringified down to HTML. This makes many parts of the codebase much easier to reason about and test, stuff like Root fragment breaks build #7 shouldn't be able to happen again.
  • Language switched to TypeScript, although I don't feel strongly about this change. I mostly made the switch because the strict typing helps me reason about code architecture when starting to build something, I wouldn't mind dropping it at this point if you're opposed to the switch.
  • Implemented support for _app.js, inspired by Next.js.
  • pages has been moved to src/pages (it makes sense if have stuff like src/components/header.js). This is a very opinionated change, I don't feel strongly about keeping it in.
  • Emulate new JSX transform benefits #9 is merged in.
  • Head doesn't use global anymore.
  • The main build function is split up and heavily refactored in the hopes of making it easier to think about.

There's still some stuff to do: I haven't had the time to add CSS processing yet (although that shouldn't be a problem) and buildPages in build.ts is still a bit too large for my liking. But even so I'm opening this issue now to let you know that this is a thing, it'd be cool to work together on getting this merged (including any changes you propose, of course).

Not creating a PR yet because I'd like to hear your opinion on this first. But fair warning, if you take too long I'll probably publish my fork on NPM; I certainly changed enough code and concepts to allow me to that. First and foremost I rewrote this because I want to use dhow for my homepage, and I've been putting that off for long enough. Please don't take this as blackmail or something like that, I just want to be able to start work on that in a reasonable timeframe (and I'll need my changes to be on NPM for that).

@kartiknair
Copy link
Owner

Wow, this is a lot of clean work, thank you so much for your interest & support on this project! I have no problem merging this rewrite in, just had a few notes on some of the mentioned changes:

min-document and associated logic is replaced with a custom (very basic) virtual DOM implementation

This is really nice, min-document isn't being maintained anymore so replacing it definitely seems like a good idea. Also allows for more freedom in implementing renderer-level features. I might have missed it but I think the jsx-runtime is missing support for the html="directly embedded inner html" attribute; however, that is simple enough to implement.

pages has been moved to src/pages (it makes sense if have stuff like src/components/header.js)

Unsure about this, leaving it as is would be non-breaking & as previously we can use the -i, --indir flag to switch to something like src/pages (see examples/portfolio). Related to this, I think leaving the CLI (i.e. command names & flags) as is would probably be better (especially considering that src/cli.ts looks to be incomplete & code from the existing CLI can be used to fill in the gaps).

#9 is merged in.

Thank you for #9, it's a very nice convenience feature that I like about Next.js.

I think that's all the suggestions/notes I have to mention. Otherwise, the rewrite looks very clean, & definitely easier to maintain!


Please don't take this as blackmail or something like that

Haha not at all, feel free to publish your fork anytime, I really don't mind.

@fs-c
Copy link
Contributor Author

fs-c commented Mar 14, 2021

Hey, thanks for the quick response! I'll open a PR in the near future (probably tomorrow) and try to incorporate some changes you suggested.

I might have missed it but I think the jsx-runtime is missing support for the html="directly embedded inner html" attribute

Yeah that's true, I forgot that html="" was a thing. I won't be able to do it today but I'll take care of it.

Unsure about this, leaving it as is would be non-breaking & as previously we can use the -i, --indir flag to switch to something like src/pages (see examples/portfolio).

Completely forgot about command line arguments, using that is definitely preferable to introducing a breaking change. I think I may have hardcoded the build function to expect a pages subdirectory in the fromPath, but that's an easy fix.

Related to this, I think leaving the CLI (i.e. command names & flags) as is would probably be better (especially considering that src/cli.ts looks to be incomplete & code from the existing CLI can be used to fill in the gaps).

Absolutely, I didn't get around to fully implementing the existing CLI behavior; I added it to the TODO.

@fs-c
Copy link
Contributor Author

fs-c commented Mar 14, 2021

One thing that I meant to look at but didn't have the time to pursue is building pages entirely in memory, as opposed to first transpiling the jsx to staging and then building the page. Mainly adding this as a note for me later on but maybe you're interested in looking into it as well.

@kartiknair
Copy link
Owner

Compiling to a staging directory might not be needed. I think the esbuild transform API would allow for directly getting a string after compiling. The part I'm missing is how we require() the compiled contents.

@fs-c
Copy link
Contributor Author

fs-c commented Mar 15, 2021

Yeah that API is what made me think of the possibility. The NodeJS VM module should allow for running a string of JS code. I was previously worried that this wouldn't be an option because require doesn't work in the VM but I forgot that esbuild can also act as a bundler so that shouldn't be a problem. I'll take a stab at it sometime this week when I have more time.

This was referenced Mar 15, 2021
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 a pull request may close this issue.

2 participants