-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Single typing file #2108
Single typing file #2108
Conversation
543e7c2
to
fa33b83
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.
Great work! I'm a big fan!
@@ -5,7 +5,7 @@ | |||
"main": "dist/rollup.js", |
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 change also means that we can–and should–move all dependencies
back into devDependencies
so that rollup is now back to its small and lean footprint!
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.
Ahh I missed that these had grown! Will do.
@diervo, @KingHenne, @jthoms1, @alan-agius4, @chadhietala as you have taken some interest in rollup's typings before, maybe you could take a look by installing rollup via
and see if this works for you? If there are no hard objections I would love to see this released soon. |
src/rollup/types.d.ts
Outdated
pluginCode?: string; | ||
} | ||
|
||
export type VERSION = string; |
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 look right, AFAIK VERSION
return a string value and thus it should be a const
and not type
.
The usage I believe is:
import { VERSION } from 'rollup';
const rollupVersion = VERSION:
Therefore the typing should be:
export const VERSION: string;
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.
Thanks
src/rollup/index.ts
Outdated
import ensureArray from '../utils/ensureArray'; | ||
import { SourceMap } from 'magic-string'; | ||
import { createAddons } from '../utils/addons'; | ||
import commondir from '../utils/commondir'; | ||
|
||
export const VERSION = '<@VERSION@>'; |
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.
Has this been removed on purpose?
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.
Well caught - yes I removed this one by mistake.
src/rollup/types.d.ts
Outdated
perf?: boolean; | ||
|
||
// deprecated | ||
entry?: string; |
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.
just a small note, for deprecations
, I suggest to use /** @deprecated */ on top of each property that is deprecated, rather than a plain comment like this.
Reason being is that if you have tslint
it can warn you that you are using a deprecated property, so do the IDE's
or any code analyse tool such as SonarCube
.
Good stuff @guybedford! I tried them out and they do work, Just felt a couple of comment. Just a small side note What I don't like in all fairness the fact that the types for |
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 also added a few notes, and most importantly when I run $(npm bin)/tsc -p . --noEmit
I get 5 compiler errors. (Maybe this should be part of the script that's run by Travis and AppVeyor?).
@@ -22,7 +22,7 @@ | |||
"posttest-coverage": "remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.json -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.lcov -t lcovonly -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped -t html -b dist", | |||
"ci": "npm run test-coverage && codecov < coverage/coverage-remapped.lcov", | |||
"prebuild": "rm -rf dist", | |||
"build": "git rev-parse HEAD > .commithash && rollup -c && tsc -p tsconfig-types.json && chmod a+x bin/rollup", | |||
"build": "git rev-parse HEAD > .commithash && rollup -c && cp src/rollup/types.d.ts dist/rollup.d.ts && chmod a+x bin/rollup", |
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 tsconfig-types.json
is now obsolete and can be removed.
@@ -5,7 +5,7 @@ | |||
"main": "dist/rollup.js", | |||
"module": "dist/rollup.es.js", | |||
"jsnext:main": "dist/rollup.es.js", | |||
"typings": "dist/typings/node-entry.d.ts", | |||
"typings": "dist/rollup.d.ts", |
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.
Since there are no files in dist/typings
and the files in typings
are not relevant anymore for the published artifact:
- the script
"pretest:typescript"
can be simplified to"rm -rf test/typescript/dist && cp -r dist test/typescript/"
and "typings/package.json.d.ts"
can be removed from"files"
.
src/rollup/types.d.ts
Outdated
@@ -0,0 +1,343 @@ | |||
import { EventEmitter } from 'events'; | |||
|
|||
// ast node types just any, unless acorn public dependency added |
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.
Hm, that's kind of a step back. As a plugin author I'm loosing information here since Program
and Node
are referenced (indirectly) in some of the hooks.
I've removed the unused package.json dependencies and scripts, and updated the dependencies to just reference types/estree and types/node generically. Thanks all for feedback so far - please try it out again and let me know any further comments. |
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 you need to update the package-lock.json
file. And there are still compiler errors (now 7). (I executed $(npm bin)/tsc -p . --noEmit
.)
src/rollup/types.d.ts
Outdated
|
||
// ast node types just any, unless acorn public dependency added | ||
export type Program = any; | ||
export type Node = any; |
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.
👍
Thanks @KingHenne, I've fixed those remaining type errors. |
"require-relative": "^0.8.7", | ||
"rollup-pluginutils": "^2.0.1" | ||
"@types/estree": "0.0.38", | ||
"@types/node": "*" |
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 results in npm WARN The package @types/node is included as both a dev and production dependency.
. But maybe you still want it to be this way so that the dependency can specify that any version works? Is that even true, i.e. is any version of @types/node
compatible with the rollup 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.
Thanks, I missed the devDependency here actually (and for some reason my npm wasn't giving a warning on this).
Well EventEmitter has been there since the very start in Node so yes, any version. The star is so that it can work alongside any version a user may install.
@@ -148,7 +134,7 @@ export default class Module { | |||
chunk: Chunk; | |||
|
|||
ast: Program; | |||
private astClone: Program; | |||
private astClone: ESTree.Program; |
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 confuses me. Shouldn't ast
and astClone
have the same type?
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.
There are really three things here not two - ast
, astClone
and enhancedAst
. ast
doubles as both EStree and the enhanced form, while the cache API for ast and astClone relies only on the EStree interface.
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.
Interesting. Should ast
be defined as Program & ESTree.Program
then?
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.
Program is a superset of ESTree.Program, so that would be the same thing here I think.
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 that is the confusing part to me, because the ast
that is passed to setSource
is defined as ESTree.Program
(via ModuleJSON
). So assigning it to a field of type Program
, which is supposed to be a superset, is technically an error then which is hidden by the type cast, right?
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 way to think about this is that Module.ast
when first assigned is an ESTree.Program
but transitions to a Rollup.Program
during the enhance call (initiated by analyse). When Module.ast
is exported, cloned or copied, even though it is a Rollup.Program
it is only needed to be a ESTree.Program
through the cache interfaces, as later on enhance will be called again on it.
That's why I say it's really two things - Module.enhancedAst
and Module.ast
.
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.
Note this problem is only temporary as I am currently working on a branch that will finally separate these two types. In the end, Module.ast
will be only a Program
while the Program
constructor will receive an ESTree.Program
as parameter.
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.
Hm, that's tricky. In my opinion, it should really be ESTree.Program | Program
then and consequently all five places with compiler errors that result from this change should get a guard so that this won't lead to a cryptic runtime error in the future when someone changes how and when the different methods in Module
are called for some reason, e.g.:
if ('bind' in node) {
node.bind();
}
or:
if (!('bind' in node)) {
throw new Error('Unexpected AST node found. ...');
}
EDIT: Just saw the comment by @lukastaegert. This is then fine for me. 🙂
this.astClone = ast; | ||
} else { | ||
// TODO what happens to comments if AST is provided? | ||
this.ast = <any>tryParse(this, this.graph.acornParse, this.graph.acornOptions); | ||
this.astClone = clone(this.ast); | ||
this.ast = <Program>tryParse(this, this.graph.acornParse, this.graph.acornOptions); |
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.
Continuing my previous question: Isn't this.graph.acornParse
returning an ESTree.Program
?
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.
Yes, but then we mutate this later through enhance
, and TypeScript doesn't support mutating a type!
Sorry for the wait, will merge this today. |
This fixes up the main Rollup API typings to be a single curated
types.d.ts
file, which other internal typings then import from.It does mean maintaining some exported dependency typing boundaries a little. The only real issue is typings of AST Nodes not passing through, but this doesn't seem to much of an issue (I've just aliased these any for now).
What's also quite nice is the typing catches the difference between code splitting and non code splitting cases an outputs, so gives correct IDE hints through the build.
Fixes #2008 and replaces #2094.