-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Generate typings for code in app-util #121
Conversation
@@ -166,11 +167,11 @@ class Watcher extends EventEmitter { | |||
entry: 'main/runtime/navigation.js', | |||
deps: {} | |||
}, | |||
files: 'build', |
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.
i can't find any reference to this option in app-util
and the types for opts do not allow it.
packages/app-utils/cpr.js
Outdated
|
||
// recursive copy file to make scripts in package.json cross-platform | ||
|
||
function copyRecursiveSync(src, dest) { |
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 code actually exists inside app-utils
itself
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.
you mean in src/files
? I didn't know that, but that's inside a .ts
file so I can't easily call it. I could import it from the compiled code, but then the build needs to have run before (the tsc
here only outputs typings; it doesn't build). do you think that's better? I put the file in the root in analogy to rimraf
which is also used in a package.json
script.
@ehrencrona looks like this one will need to be rebased |
We already have |
@benmccann I am setting |
aab5112
to
e4dd700
Compare
rebased |
It does look like it's supported. However, the Rollup TypeScript plugin has lots of weird usage quirks and in this case it looks like you also need to set |
@ehrencrona I merged #134 so you can rebase this one |
e4dd700
to
7c425f2
Compare
@benmccann I've rebased. Update: adding I added the type generation and copying the types to the rollup build now. This way it will also work when running |
|
@@ -6,3 +6,4 @@ node_modules | |||
/http | |||
/renderer | |||
/types |
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.
do we need this if we're outputting to /build/types
?
@@ -6,3 +6,4 @@ node_modules | |||
/http | |||
/renderer | |||
/types | |||
/build |
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.
I think I saw one of the packages was outputting to /dist
at one point? Should we standardize on that?
Also, I wonder about the fact that this is outputting the types to /build/types
but the other files to /common
, /files
, etc. Should those also be under /build
or /dist
? I think that'd be a lot easier to manage. I was having a hard time with the way it's setup now with output going all over the place. It's hard to tell what's output and what's not. E.g. we generate some .d.ts
files but there are other ones that are checked in and I can't easily tell which is which
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.
The tsconfig
builds the types to build/types
(which contains e.g. files/index.d.ts
etc) and then copies the to the root (whereby build/types/files/index.d.ts
becomes files/index.d.ts
and therefore winds up next to index.mjs
as it should).
I couldn't get it working building straight to .
; it doesn't seem to like the output directory to be a parent directory of the src directory (No inputs were found [...] Specified 'include' paths were '["src/**/*"]' and 'exclude' paths were '["."]'.
). Maybe there's a way to fix that?
I agree that having all the outputs in the root is confusing and it would be nice to move them (to dist
), but that feels like a different issue from generating typings. I suppose they could be moved with the exports
tag in package.json
but that would require everyone to use a modern npm. Otherwise I don't know of any way of doing it without changing the exported module names.
As long the complete build isn't to dist
I think build
is a better name for intermediary build artefacts than can be wiped.
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.
When I say it wasn't working, I mean that the projects wouldn't all compile.
Oh, I notice now there are compiler errors due to the types in other projects. Sorry, I hadn't noticed before (though I think some are fixed by other PRs). Fixing them.
2223042
to
7e711d5
Compare
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.
looks good to me other than needing to be rebased. thanks for being patient with this one
7e711d5
to
f620a67
Compare
rebased. no worries, as long as there's feedback coming in it's a good process :) |
@ehrencrona it looks like this one will need to be rebased again since your other PR was merged |
3377990
to
408a048
Compare
and rebased |
This PR builds on #116 and adds typings for the code in
app-utils
(#116 only exported the manually defined types; code such as therender
method was still not typed to consumers outside the package).The
buildTypes
script defined inpackage.json
builds the types which are put alongside the compiled code (e.g./renderer
or/files
). I couldn't gettsc
to build directly to the right location so it builds tobuild/types
and then copies the result over.The manually defined typings that were previously in
src/types.d.ts
would have looked weird if they were copied to the root so I moved those intosrc/types/index.d.ts
. This doesn't affect imports and the "built" location becomestypes/index.d.ts
which is in line with the compiled code (though still somewhat confusing).I didn't actually make the types build as part of the
build
step out of concern for build times. The build will work even without them, but building types manually risks creating confusion if you have out-of-date typings (which can fail the build). It runs in 5.5 seconds seconds on my Macbook Air 2019. Should we add it to the build?