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

Add new NPM script to build using tsc to generate types and update package.json to indicate path to types #6

Closed
wants to merge 6 commits into from

Conversation

mattmazzola
Copy link
Contributor

I'm not sure if this would be useful but I was playing around with have two build outputs. Using typescript to generate types build while keeping the existing esbuild since it does minification and makes this non-breaking.

I'm not very familiar with the new package.exports config but I think this might help work towards resolving #3

@mattmazzola mattmazzola changed the title Add new NPM script to build using Typescript and update Basic demo to module output Add new NPM script to build using tsc to generate types and update package.json to indicate path to types Nov 6, 2022
@mattmazzola
Copy link
Contributor Author

Updated to output ESM files and CommonJS files as well as types in separate directories. I'm not sure if it would be needed but it was the idea.

image

@dagnelies
Copy link
Collaborator

dagnelies commented Nov 7, 2022

I think this would break the demo which imports the minified js directly using a relative path.
I know looks a bit odd, but this is really nice to test locally if "everything is fine" with the minified thing.

I think I would have to fix the CDN / README / example links in a few places too, also on the main site and a few other places. It would probably be easier to just add the types directly in dist.

That said, I was pondering about making a bigger refactoring. It's described in #7, which would need further compiling adjustments, so I'd rather make it in one swoop with it. Nevertheless, the indications provided are very useful, thank you very much for the assistance.

@mattmazzola
Copy link
Contributor Author

this would break the demo which imports the minified js directly using a relative path.

Hmm, it was intended to be non-breaking but maybe I missed something. Notice the changed files, dist/webauthn.min.js is no listed so it was not moved or changed. Also the package.main wasn't changed so I think the demos and package consumption works as before, but now has types.

this is really nice to test locally if "everything is fine" with the minified thing

Yes, I remember you mentioned that on a previous issue which is why I didn't mean to change it. I think using npm link or local package install is a more flexible method, but I think another reason not to change is the way you are serving on unpkg. Normally the entire dist folder would be ignored, but you have dist/webauthn.min.js tracked and committed

probably be easier to just add the types directly in dist

I didn't understand what you meant here. The picture I showed above, the types are in the dist folder. Even if you moved the types to the root of dist I think you would still have to use custom urls because the entry point of the types would be dist/index.d.ts not dist/webauthn.min.d.ts which would be the default and it doesn't exist.

It's described in #7, which would need further compiling adjustments, so I'd rather make it in one swoop with it.
the indications provided are very useful, thank you very much for the assistance.

Ok, yea. I was mostly sending this to give ideas. If you decide to close the PR and not merge it since you are doing the larger refactor that is ok with me.

@dagnelies
Copy link
Collaborator

Thanks for the MR ...since the build process is a bit tricky, I ended up repeating your steps one by one as separate commits. Basically, it's now "merged" with slight differences along the way. Thanks.

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.

None yet

2 participants