-
Notifications
You must be signed in to change notification settings - Fork 185
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
Use Rollup and Babel to create builds #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be very careful using babel to compile our TypeScript.
I've seen cases where it doesn't handle certain valid code that the TypeScript compiler handles just fine.
And on the other end babel sometimes (depends on config) supports code that isn't (or not yet) valid TypeScript. I pointed out some proposal
settings we have in the babel config.
The main issue here is that we now use two tools to verify/compile our code and they aren't 100% compat:
- TypeScript compiler for in-editor checks and to generate type definitions
- Babel for transpilation time checks and to generate JavaScript
Also the need to remove more circular imports than was necessary previously should ring alarm bells.
It got fixed now, but this may become a pain in the future if the babel compiler has problems in more cases regarding circular imports than even the TypeScript compiler has.
Have you considered the much faster and more compatible option https://github.com/evanw/esbuild? I've set it up with rollup in the past, so may be a better option for us.
Please don't merge until we discuss the above + alternatives.
src/types/IdentityDriver.ts
Outdated
@@ -13,8 +13,8 @@ export abstract class IdentityDriver extends Driver implements IdentitySigner { | |||
return nacl.sign.detached.verify(message, signature, this.publicKey.toBytes()); | |||
} | |||
|
|||
public equals(that: Signer | PublicKey): boolean { | |||
if (!(that instanceof PublicKey)) { | |||
public equals(that: { publicKey: PublicKey } | PublicKey): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really belong in a build system update PR. Also I already fixed this, see #94 , so we'll just run into unnecessary conflicts.
@thlorenz Sure I’m open to discuss and try alternatives to babel but I’ve got a few comments.
EDIT: Checking out esbuild today and it looks like it also has TS caveats like babel. |
I cannot remember details but I ran into this in the past. It can be pretty annoying and time intensive to figure those out. Babel is great when it works until it doesn't and then you wanna tear your hair out (I've been there).
If our code compiles without it then we should
That one is not a huge priority as I can work around the conflicts if this one gets merged first. I was just pointing out that that change has nothing to do with the build system change and ideally should be part of a separate PR. TL;DR;I want to have a go at replicating this setup with esbuild which to my knowledge is more compat with the TypeScript compiler afaik. Also we'll see a huge speed improvement most likely. The only reason that the vite framework (for vue) isn't completely switching over is since it doesn't support CSS bundling/code splitting fully yet. However giving this a go should be fairly simple to do since the needed plugins exist. LMK what you think. EDIT: just saw your edit. From reading those points it seems like TypeScript here is actually not fully ESM compat and thus esbuild outputs something different. Also it appears that using I suppose we'll look at both options and see which one we like most. |
I agree that esbuild is a good alternative. I decided to go with babel because of web3.js using it and because it's been around for much longer but I'm not against trying it. If possible, I'd like to try using esbuild on its own though rather than through a rollup plugin. Is there any reason we should keep using rollup with esbuild? I might create a new branch from this PR (since I've removed some circular imports) to try it out. Let me know what you think. |
There is no reason to use rollup if esbuild provides all we need on its own. |
I've been fighting all morning with esbuild and I'm not sure it's quite ready for library development I'm afraid. I tried both as a standalone solution and with Rollup. Lot's of open issues for pretty basic things that I just can't do as a standalone or have to create various workarounds in rollup which makes the build complex and pretty hacky. Here are some examples of esbuild limitations:
I've managed to get to a stable point with Rollup + Babel which is not too complex and works really well at exporting libraries just the way we want them. I think we should merge that in and revisit this at a later stage when esbuild is more mature and tailored for libraries that want to preserve their modules. |
The TypeScript compile has done a good job so far and has kept things simple but we've reached the limit of things we can do with it. This includes exporting ES Modules that work on end-user apps out-of-the-box without any workarounds.
The new builds include: