Skip to content
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

three: Make THREE non-global #15861

Merged

Conversation

efokschaner
Copy link
Contributor

@efokschaner efokschaner commented Apr 14, 2017

@gyohk @florentpoujol @SereznoKot @omni360 @ivoisbelongtous @piranha771 I'd appreciate if any of you could review. Thanks in advance.

I was running into duplicate identifier issues as a result of using npm link, as described in this issue microsoft/TypeScript#11916 (comment) . Following the advice of @mhegazy in that thread, we should make the definitions not establish a global THREE namespace by default.

Now if you import three as a module you do not get a global THREE namespace defined. You will still get a global namespace if you use a /// reference directive. @mrdoob this change might interest you, I know sometimes people reach out to you regarding using three.js with TypeScript.

99% of the change is just removing the namespace THREE nesting. The exceptions are as follows:

  • move core THREE members from types/three/index.d.ts into types/three/three-core.d.ts
  • consolidation of contributors and metadata into types/three/index.d.ts
  • consolidation of references for tests in to types/three/test/references.d.ts
  • last line of types/three/detector.d.ts (export as namespace)
  • last line of types/three/index.d.ts (export as namespace)
  • moved some declarations from types/three/test/three-tests-setup.ts to types/three/three-examples.d.ts These were low quality definitions that were only in tests before, but they're not wrong afaik, they just use any.

This has potential to be breaking for users of three definitions who rely on it being global, but global use is still working as can be seen from the tests so it may depend on how they're importing the definitions.

As an example of something that could break, openjscad was augmenting the THREE namespace but that requires using declare module now, instead of declare namespace. @danmarshall Can you review my fix to openjscad?

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: No API changes
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "../tslint.json" }. we'll get there one day :)

@dt-bot
Copy link
Member

dt-bot commented Apr 14, 2017

types/three/detector.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/index.d.ts

to authors (@gyohk @florentpoujol @SereznoKot @omni360 @ivoisbelongtous @piranha771 Kon (account can't be detected)). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/test/references.d.ts

Checklist


types/three/three-FirstPersonControls.d.ts

to author (@s093294). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-canvasrenderer.d.ts

can't parse definition header...


types/three/three-colladaLoader.d.ts

can't parse definition header...


types/three/three-copyshader.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-core.d.ts

Checklist


types/three/three-css3drenderer.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-ctmloader.d.ts

to author (@omni360). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-editorcontrols.d.ts

to author (@qszhusightp). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-effectcomposer.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-examples.d.ts

Checklist


types/three/three-maskpass.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-octree.d.ts

to author (@omni360). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-orbitcontrols.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-orthographictrackballcontrols.d.ts

to author (@Pro). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-projector.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-renderpass.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-shaderpass.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-trackballcontrols.d.ts

to author (@gyohk). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-transformcontrols.d.ts

to author (@Pro). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-vrcontrols.d.ts

to author (@nakakura). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/three/three-vreffect.d.ts

to author (@nakakura). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@danmarshall
Copy link
Contributor

@efokschaner I will review the openjscad and get back to you, but don't wait for me - go ahead with your change. OpenJScad has been refactored so I think there won't be too much dependency on this. Thanks

@florentpoujol
Copy link
Contributor

It's been so long since I worked with THREE or Typescript that I can't judge the relevance of this change now.

But if it fix some things while not breaking existing uses, then go for it I guess...

@RyanCavanaugh RyanCavanaugh added Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. Author Approved labels Apr 18, 2017
@efokschaner
Copy link
Contributor Author

Thanks for the vague thumbs up @florentpoujol
@RyanCavanaugh I've merged the conflicts and it should be good to go again. Thanks!

@RyanCavanaugh RyanCavanaugh removed the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Apr 19, 2017
@RyanCavanaugh RyanCavanaugh merged commit c91f8ff into DefinitelyTyped:master Apr 19, 2017
@efokschaner efokschaner deleted the make-three-non-global branch April 19, 2017 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants