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

docs: add demo #43

Merged
merged 5 commits into from
Sep 6, 2024
Merged

docs: add demo #43

merged 5 commits into from
Sep 6, 2024

Conversation

Gregoor
Copy link
Collaborator

@Gregoor Gregoor commented Sep 6, 2024

Hey @brettz9,

I finally made time to take your #28 as inspiration and spin up a demo based on your idea, with tooling I'm more familiar with.

This also changes a few other things:

  • src/ => lib/
  • uses pnpm workspace so that demo/ can easily depend on lib/
  • adds a GitHub workflow to deploy to Pages

Would be happy to get a review from you this time around!

Copy link
Contributor

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

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

I can't really assess the React code, as I'm not too familiar with React, but the demo looks great.

The rest LGTM, except:

  1. My build script and change of file extensions to ".js" should be necessary to allow the exported code to work effectively in the browser for importing projects (at least without adding their own build steps).

I know many TS folks prefer to keep the ".js" extensions off, but this doesn't work in the browser (unless forcing users to roll up the code themselves or use import maps in a non-recommended manner. With my build script approach, the only entry one must add to an import map is for zod (see my demo/index.html <script type="importmap">), allowing the browser to discover where "zod" in import {z} from "zod" is to be found. TypeScript, moreover, deliberately doesn't provide an approach to change the outputted file extensions, so one cannot use TS config to do this.

My use of exports in package.json is not required but does help encapsulate by limiting one's public API to the files therein (and also allow users to avoid adding the dist in the path if they want to reach into and import a specific file you do want to export).

  1. You might want to add "type": "module" so you can just use .js instead of .mjs (and use .ts instead of .mts) (and also future-proof the code thereby as Node says it may swap the default from commonjs to module at some point; at least one should make explicit "type": "commonjs" if not using modules)

  2. My husky file changes were recommended

Let me know if I can clarify anything further.

@Gregoor
Copy link
Collaborator Author

Gregoor commented Sep 6, 2024

Thanks for the review!

Those are interesting points, I have to admit I have not properly considered browser non-bundler imports. In looking at what other libs used, I stumlbed over tsup, which I might want to investigate for that.

The husky changes also sound good.

I'll look at both in another context, as I think the demo can live without either. Especially the browser-compatibility issues I will want to solve before we declare zodex 1.0 (which I think is imminent).

@Gregoor Gregoor merged commit 6cd62f5 into main Sep 6, 2024
10 checks passed
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.

2 participants