Fix build #48
Fix build #48
Conversation
Note: vendor > hypothesis is missing shared types
Emit definition types + module resolution in testapp
Remove exports from packages as they wreak havoc AS-IS
Shared tests needs cjs ext cos type module
Vite’s Library mode can’t export static assets and defaults to Base64. So we currently need a plugin to handle that: vitejs/vite#3295 Note: to do things properly, we should also bundle the fonts’ licenses.
Possibly a review of what and how should be exposed/exported is warranted. But at the moment, it offers a baseline while making packages imports more consistent.
|
OK so I’m not expecting to commit more changes at the moment, and I guess the PR is ready for review – and additions for Tried to follow |
| @@ -46,7 +46,7 @@ export class FrameComms { | |||
| } catch (error) { | |||
| this.channelId = mid(); | |||
| } | |||
| this.gc = setInterval(() => { | |||
| this.gc = window.setInterval(() => { | |||
There was a problem hiding this comment.
Why did you add window (here and other places)?
There was a problem hiding this comment.
That looked like the easiest way to fix TypeScript complaining of the number type as it thought it could be NodeJS.Timer.
An alternative would be ReturnType<typeof setTimeout> but considering navigator is only intended to be used in browsers, it seemed like a more obvious option. Although I’d prefer to reach some agreement, obviously.
There was a problem hiding this comment.
It seems like having gc be of type number makes sense -especially since clearInterval takes a number -and that window. facilitates that. Just my two cents. :)
There was a problem hiding this comment.
@JayPanoz OK, thanks for explaining. The reason why I ask is technically the frame communications part of the navigator is something that could be re-used in a non-browser scenario (like Thorium's Node.JS) to communicate with an iFrame. But that's a very theoretical case.
There was a problem hiding this comment.
@chocolatkey Thanks for pointing that out. Removed it from navigator-html-injectables and changed the type so that it adapts to ts-config, kept it in navigator.
That way it can be used with Node
|
Thanks for your feedback. 🙏 |
This PR should fix the
buildscript for all packages/workspaces in the repository, and make shared models testable again.It takes care of the following issues:
For consistency, ViteJS is now used across the board and
tsconly emits definition types. Note the existing Github Workflow targetingSharedwas slightly edited so checks can be passed, it doesn’t handle other packages ATM.Note Vite will emit a warning because of ReadiumCSS inlined in Navigator but it hasn’t any negative effect:
As a sidenote, to do things properly the fonts’ license that are present in ReadiumCSS should be output with static assets in
distso this is something to keep in mind.