-
Notifications
You must be signed in to change notification settings - Fork 113
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
Ported to Typescript #22
base: gh-pages
Are you sure you want to change the base?
Conversation
...will convert to Webpack, later, because rollup seems to have problems with exporting the *.d.ts file from Typescript.
...now we have several build results, i.e. one UMD module, one CommonJS module and one ES module. Adjusted README to new output path names.
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.
First of all, this is great - thanks for doing this. I think only very small changes stand between the current state and merging this PR.
I do have some higher level feedback / questions:
- I understand why individual developers might want to use yarn, but I don't see a strong reason to force people to use it. Everyone should have npm and they have essentially the same features and performance these days. Alternatively, yarn could be installed as a dev dependency so people can just run
npx yarn
. - It would be useful to add some guidance to the README regarding how non-TypeScript users could import only part of the project. Alternatively, it'd be nice if the build process emitted separate files for each source file (in addition to a combined file) so that non-TypeScript users could use parts of the project without needing a TypeScript compiler / bundler.
- It seems that build files are omitted from this PR; how would you then propose that the demo remain functional, and how would you propose doing releases of this library? (Understanding this is a merge blocker.)
- It looks like source maps aren't emitted? That seems like a significant regression, probably a merge blocker. Should be a small change to add that.
- Is the idea with ESM that a consumer will use Rollup or something like it, with their own custom config, to consume the imports, bundle everything up, and eventually minify it? As is, the UMD version is a much smaller file than the ESM version.
.gitignore
Outdated
@@ -1,2 +1,3 @@ | |||
node_modules | |||
package-lock.json | |||
dist/ |
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.
Missing trailing newline?
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.
First of all, this is great - thanks for doing this. I think only very small changes stand between the current state and merging this PR.
Cool.
I do have some higher level feedback / questions:
- I understand why individual developers might want to use yarn, but I don't see a strong reason to force people to use it. Everyone should have npm and they have essentially the same features and performance these days. Alternatively, yarn could be installed as a dev dependency so people can just run
npx yarn
.
OK, I've ported it back to npm.
- It would be useful to add some guidance to the README regarding how non-TypeScript users could import only part of the project. Alternatively, it'd be nice if the build process emitted separate files for each source file (in addition to a combined file) so that non-TypeScript users could use parts of the project without needing a TypeScript compiler / bundler.
I will have to think about this.
- It seems that build files are omitted from this PR; how would you then propose that the demo remain functional, and how would you propose doing releases of this library? (Understanding this is a merge blocker.)
The demo simply runs as before. I changed almost nothing in the demo files. Just try
npm run build
npm run serve
- It looks like source maps aren't emitted? That seems like a significant regression, probably a merge blocker. Should be a small change to add that.
Yes, I'll re-enable them.
- Is the idea with ESM that a consumer will use Rollup or something like it, with their own custom config, to consume the imports, bundle everything up, and eventually minify it? As is, the UMD version is a much smaller file than the ESM version.
Exactly. People who use modules will have their own bundler, like webpack or similar. They will run their own minifier, of course.
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.
Re-enabled sourcemaps, now.
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 demo simply runs as before. I changed almost nothing in the demo files. Just try
npm run build
npm run serve
Since the artifacts produced by npm run build
are not tracked by git, the demo will not run on GitHub Pages. (Previously the artifacts were tracked in the build
folder, whereas with this PR they're in the dist
folder which is in .gitignore
.) Similarly, I think library releases would require an additional packaging step so that consumers would not have to build this project in order to make use of it as a dependency.
|
||
export type HeightmapFunction = ((g: Vector3[], options: TerrainOptions) => void); | ||
|
||
export interface TerrainOptions { |
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 interface makes all properties required, but only a few are required for each function that uses TerrainOptions. A couple places do use Pick
or Partial
, but many uses don't. In no case are all properties required, and in some cases all properties are optional.
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 guess I will need you help, here. I wasn't able to always figure out if the options were needed or not. Sometimes, when I used too much Pick or Partial, I ran into a dead end because I could not make it work. Most of the places where TerrainOptions is used are called from Terrain() anyway so that the options all have default values.
Can you help here, please?
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.
If you change export interface TerrainOptions {
to export type TerrainOptions = Partial<{
and recompile, you will get 164 errors, each listing a property that should be available. For example, GaussianBoxBlur
will complain that it's missing options.xSegments
and options.ySegments
.
I acknowledge that this is a substantial update. As a compromise, how about we just make the most commonly used properties required?
export interface TerrainOptions {
after?: ((v: Vector3[], options: TerrainOptions) => void) | null;
easing?: EasingFunction;
heightmap?: HTMLCanvasElement | ImageBitmap | HeightmapFunction;
material?: Material | null;
maxHeight: number;
minHeight: number;
optimization?: Optimization;
frequency: number;
steps?: number;
stretch?: boolean;
turbulent?: boolean;
useBufferGeometry?: boolean;
xSegments: number;
xSize: number;
ySegments: number;
ySize: number;
}
export * from './analysis'; | ||
export * from './basicTypes'; | ||
export * from './brownian'; | ||
export * from './core'; |
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.
THREE.Terrain.Terrain
is awkward, and unnecessary I think. It could potentially be improved with something like this (untested):
import * as T from './terrain';
import { Analyze } from './analysis';
import { Brownian } from './brownian';
// ...
export function Terrain(...args) {
return T.Terrain(...args);
}
export namespace Terrain {
export const Analyze = Analyze;
export const Brownian = Brownian;
// ...
}
I'd be interested in alternatives you may have considered.
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.
Hmmm ... not sure about this one. Microsoft wrote a doc page that discourages using modules and namespaces in the same file. I tried similar stuff in the past and I always ran into some kind of absolute weirdness. It really seems as if Typescript does not like mixing up modules and namespaces. However, it was too long ago so that I do not remember what happened exactly.
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.
As far as I'm aware, the whole purpose of namespaces is to merge declarations like this. Nothing on the page you linked that I can see warns against combining them. Regardless, I don't care so much how it's accomplished, I just think Terrain.Terrain
is awkward and it should be possible to construct an API signature that doesn't repeat itself 🙂
@mattes3 are you planning on making any updates? |
Oops, I completely missed your feedback because I'm currently under a pile of other things. Will have a look at this over the weekend. |
No worries |
OK, so most of this stuff is done, some things are still remaining. I'll have to get some sleep, first. Maybe you can have a look and tell me what you think. |
Responded to some threads - the other changes look good. Thanks for the updates. As an FYI, I am going on vacation tomorrow for about 10 days, so I may not be able to answer questions during that time. |
I cannot proceed with this. Does anyone like to take over the work? |
@IceCreamYou are you able to merge this? |
No, this is not in a releasable state. The biggest issue is that the new design does not support non-TypeScript users. |
As far as I remember, this is not true. I invested a lot of time in 2019 to make sure that it still runs in plain JS. |
Sorry, I was imprecise. The biggest problem is in the packaging:
In other words, sure, consumers can use the library without TypeScript... but only if they use TypeScript to compile it first. There are a couple other smaller things I'd like to see addressed as well, most notably the API signature change to |
This solves #18.