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

module: commonJS produces require of .ts file? #8031

Closed
cfv1984 opened this issue Apr 12, 2016 · 15 comments
Closed

module: commonJS produces require of .ts file? #8031

cfv1984 opened this issue Apr 12, 2016 · 15 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@cfv1984
Copy link

cfv1984 commented Apr 12, 2016

TypeScript Version:

Version 1.8.9

Code

// A self-contained demonstration of the problem follows...
//in tsconfig.json
{
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5"
  }
}

// in ../Util/Sanitizer.ts
export class Sanitizer{
  sanitize: function(str){/*various string cleanups*/}
}

//in ../Util/Util.ts
import {Sanitizer} from "./Sanitizer/Sanitizer";

export default {
    Sanitizer: Sanitizer
};

//in App.ts
import * as Util from "../Util/Util.ts";

Util.Sanitizer.sanitize("String");

Expected behavior:

//In App.js
var Util = require("../Util/Util.js");
Util.Sanitizer.sanitize("String");

Actual behavior:

//INVALID output for app code.
var Util = require("../Util/Util.ts");

Util.Sanitizer.sanitize("String");

Alternatives were tried

Without success. Doing

//in ../Util/Util.ts
export = {
    Sanitizer: Sanitizer
};

Yields

Module "project/path/to/Util/Util.ts" resolves to a non-module entity and cannot be imported using this construct.

When running tsc project/path/to/App.ts

@vladima
Copy link
Contributor

vladima commented Apr 12, 2016

extension should not be necessary, will it work if you use just require("../Util/Util")?

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Apr 12, 2016
@cfv1984
Copy link
Author

cfv1984 commented Apr 12, 2016

Removed the '.ts' part, and now I get
Property 'sanitize' does not exist on type 'typeof Sanitizer'.

Declaring it as static makes it work again. Which wasn't inmediately apparent, though. Question is now, why does the incorrect behavior -not translating all .ts into .js- persist? That is something I'd like to look into, if there was some kind of guide on looking for this kind of things.

Also, why isn't the tooling picking up that I'm calling something statically that exists as a normal method?

@vladima
Copy link
Contributor

vladima commented Apr 12, 2016

  1. sanitize property on the class Sanitizer should be static since you are accessing it from the constructor function
  2. export default ... should be consumed as import utils from ...

Sample below works without any errors.

vladima@vladima-ux31a:~/sources/playground/cjs$ tree
.
├── src
│   ├── app.ts
│   ├── package.json
│   └── tsconfig.json
└── utils
    ├── sanitizer.ts
    └── utils.ts

2 directories, 5 files
vladima@vladima-ux31a:~/sources/playground/cjs$ cat utils/sanitizer.ts 
export class Sanitizer{
  static sanitize (str) {}
}
vladima@vladima-ux31a:~/sources/playground/cjs$ cat utils/utils.ts 
import {Sanitizer} from "./sanitizer";

export default {
    Sanitizer: Sanitizer
};
vladima@vladima-ux31a:~/sources/playground/cjs$ cat src/app.ts
import util from "../utils/utils";

util.Sanitizer.sanitize("String");

vladima@vladima-ux31a:~/sources/playground/cjs$ npm install typescript@next --save
npm WARN saveError ENOENT: no such file or directory, open '/home/vladima/sources/playground/cjs/package.json'
/home/vladima/sources/playground/cjs
└── [email protected] 

npm WARN enoent ENOENT: no such file or directory, open '/home/vladima/sources/playground/cjs/package.json'
npm WARN cjs No description
npm WARN cjs No repository field.
npm WARN cjs No README data
npm WARN cjs No license field.
vladima@vladima-ux31a:~/sources/playground/cjs$ ./node_modules/.bin/tsc -p src
vladima@vladima-ux31a:~/sources/playground/cjs$ 

@cfv1984
Copy link
Author

cfv1984 commented Apr 12, 2016

@vladimir sorry but no it's not "working". Typescript is
a) leaving untranslated Typescript in the result JS and
b) complaining about the wrong category of thing going on.

Can anyone point me on a good guide to try and make this stop happening?
On 12 Apr 2016 17:25, "Vladimir Matveev" [email protected] wrote:

  1. sanitize property on the class Sanitizer should be static since you
    are accessing it from the constructor function
  2. export default ... should be consumed as import utils from ...

Sample below works without any errors.

vladima@vladima-ux31a:~/sources/playground/cjs$ tree.
├── src
│ ├── app.ts
│ ├── package.json
│ └── tsconfig.json
└── utils
├── sanitizer.ts
└── utils.ts

2 directories, 5 files
vladima@vladima-ux31a:/sources/playground/cjs$ cat utils/sanitizer.ts export class Sanitizer{
static sanitize (str) {}
}
vladima@vladima-ux31a:
/sources/playground/cjs$ cat utils/utils.ts
import {Sanitizer} from "./sanitizer";
export default {
Sanitizer: Sanitizer
};
vladima@vladima-ux31a:~/sources/playground/cjs$ cat src/app.ts
import util from "../utils/utils";

util.Sanitizer.sanitize("String");

vladima@vladima-ux31a:~/sources/playground/cjs$ npm install typescript@next --save
npm WARN saveError ENOENT: no such file or directory, open '/home/vladima/sources/playground/cjs/package.json'
/home/vladima/sources/playground/cjs
└── [email protected]

npm WARN enoent ENOENT: no such file or directory, open '/home/vladima/sources/playground/cjs/package.json'
npm WARN cjs No description
npm WARN cjs No repository field.
npm WARN cjs No README data
npm WARN cjs No license field.
vladima@vladima-ux31a:/sources/playground/cjs$ ./node_modules/.bin/tsc -p src
vladima@vladima-ux31a:
/sources/playground/cjs$


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8031 (comment)

@vladima
Copy link
Contributor

vladima commented Apr 12, 2016

I've made a short sample here

@aluanhaddad
Copy link
Contributor

@cfv1984 I'm curious as to why you wish to specify the extension explicitely and also as to why you expect extension conversion to be part of the compilation process.

@cfv1984
Copy link
Author

cfv1984 commented Apr 13, 2016

What valid use cases would there be for require'ing ts files in plain js?
On 12 Apr 2016 21:48, "Aluan Haddad" [email protected] wrote:

@cfv1984 https://github.com/cfv1984 I'm curious as to why you wish to
specify the extension explicitely and also as to why you expect extension
conversion to be part of the compilation process.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8031 (comment)

@basarat
Copy link
Contributor

basarat commented Apr 13, 2016

I'm curious as to why you wish to specify the extension explicitely and also as to why you expect extension conversion to be part of the compilation process

@aluanhaddad I think its because of the JS / Weback mindset where one does require('./foo.css') and it just gets webpacked.

@cfv1984 Please do not use an extension in require for .ts files. Stuff like webpack will look for .js files automatically (and even .ts files if you have it configured https://github.com/TypeStrong/ts-loader#configuration) and TypeScript compiler will look for .d.ts,.ts,.tsx and even .js etc based on the compiler options automatically 🌹

@cfv1984
Copy link
Author

cfv1984 commented Apr 13, 2016

I'm genuinely unsure why is it such an awkward thing to want to import a
file by its actual name.
Shouldn't this be stressed further in the docs if it's so problematic?
Maybe THAT I'll be able to do rather fast
On 12 Apr 2016 22:16, "Basarat Ali Syed" [email protected] wrote:

I'm curious as to why you wish to specify the extension explicitely and
also as to why you expect extension conversion to be part of the
compilation process

@aluanhaddad https://github.com/aluanhaddad I think its because of the
JS / Weback mindset where one does require('./foo.css') and it just gets
webpacked.

@cfv1984 https://github.com/cfv1984 Please do not use an extension in
require for .ts files. Stuff like webpack will look for .js files
automatically (and even .ts files if you have it configured
https://github.com/TypeStrong/ts-loader#configuration) and TypeScript
compiler will look for .d.ts,.ts,.tsx and even .js etc based on the
compiler options automatically [image: 🌹]


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8031 (comment)

@basarat
Copy link
Contributor

basarat commented Apr 13, 2016

I'm genuinely unsure why is it such an awkward thing to want to import a
file by its actual name.

I can understand if you have a desire to do this in import foo = require('./bar.ts') because after all var foo = require('./bar.js') is valid nodejs code however it isn't very conventional.

Thats why its just about it not being supported https://blogs.msdn.microsoft.com/ericgu/2004/01/12/minus-100-points/

JavaScript modules are still pretty new in the JS lifetime and even ES6 modules come without a loader out of the box (making it effectively a must be transpiled feature). Again its unconventional to add the file extension and I doubt you'd see any big OSS project out there adding the extension. So why add a feature that is unconventional and take on the added complexity of detecting if there is an extension and removing it if there is 🌹

@aluanhaddad
Copy link
Contributor

@basarat that was what I was getting at. Specifying explicit extensions interferes with/attempts to bypass the TypeScript compiler's module resolution logic and it causes many loaders and build tools to break.

I work on projects where some apps are transpiled dynamically in the browser during development and some are transpiled in a node environment. The apps share code without any problems but if we were to specify the extension explicitly everything would break.

The insight into the "Webpack Mindset" is interesting. I was asking this question because I was wondering if there was some specific use case where explicitly specifying the extension provided a benefit. As it stands it seems to lower the level of abstraction, and reduce interoperability, portability and maintainability.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 13, 2016

JavaScript modules are still pretty new

They might be new for some, but they have been around a long time. CommonJS is from 2009, AMD came around 2010 which was largely based on not being able to get CommonJS to understand some of things SystemJS and the the ES6 Module Loader are only discovering now. But AMD was developed because James Burke was trying to improve the ability to load Dojo Modules and Dojo had modules back in 2006, which was based on other modular concepts. 😄

I was asking this question because I was wondering if there was some specific use case where explicitly specifying the extension provided a benefit. As it stands it seems to lower the level of abstraction, and reduce interoperability, portability and maintainability.

Any use cases are a false economy because of what you rightly point out, it reduces abstraction and portability. It is often the mindset of those who are used to dealing with fixed filesystem resources which are directly addressable (server side mindset) and not recognising the reality where resources are remote and may not have a static physical location.

@cfv1984
Copy link
Author

cfv1984 commented Apr 13, 2016

@Kitson at this point I can conclude community consensus says the tsc
offline compiler does not and will not translate single file imports of its
own file type in this particular construct.

Which is I guess OK, I mean, it appears to be such a cornerstone of the
language to freak people Iike @basarat to even want to do this. Regardless
of the implications of leaving garbage in the generated JS, I'll save it in
our internal KB and ensure we don't do that any more

Now, why isn't there an error when I do this? And how would one go and make
tsc give an error here?

Also, back in my original post, there is also the issue of my getting an
error that has nothing to do with the class of problem I'm having -bad
access modifiers being reported as a missing property-. This is also very
odd as error reporting is usually pretty good.
Can anyone please point me to where this things get checked? Mere grepping
hasn't been too helpful.
On 13 Apr 2016 08:18, "Kitson Kelly" [email protected] wrote:

JavaScript modules are still pretty new

They might be new for some, but they have been around a long time.
CommonJS is from 2009, AMD came around 2010 which was largely based on not
being able to get CommonJS to understand some of things SystemJS and the
the ES6 Module Loader are only discovering now. But AMD was developed
because James Burke was trying to improve the ability to load Dojo Modules
and Dojo had modules back in 2006, which was based on other modular
concepts. 😄

I was asking this question because I was wondering if there was some
specific use case where explicitly specifying the extension provided a
benefit. As it stands it seems to lower the level of abstraction, and
reduce interoperability, portability and maintainability.

Any use cases are a false economy because of what you rightly point out,
it reduces abstraction and portability. It is often the mindset of those
who are used to dealing with fixed filesystem resources which are directly
addressable (server side mindset) and not recognising the reality where
resources are remote and may not have a static physical location.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8031 (comment)

@mhegazy
Copy link
Contributor

mhegazy commented Apr 13, 2016

Issue #4595 tracks this request.

@mhegazy mhegazy closed this as completed Apr 13, 2016
@basarat
Copy link
Contributor

basarat commented Apr 14, 2016

why isn't the tooling picking up that I'm calling something statically that exists as a normal method

And

there is also the issue of my getting an
error that has nothing to do with the class of problem I'm having -bad
access modifiers being reported as a missing property-

Here is a simplification of the code sample:

class Sanitizer {
  sanitize = function(str){/*various string cleanups*/}
}
Sanitizer.sanitize; // Error : Property `sanitize` does not exist on type `typeof Sanitizer`. 

I am sure by now you understand why this is an error (you probably wanted it to be a static member). So you have a desire to get a better error reporting.

But there is a tradeoff of complexity here (a performance hit of checking if calling new on the type would yield something that might have that method). Again even then providing this hint is also tricky Perhaps you want to call a member sanitize on an instance or Perhaps you meant to make the method static and even when the user change the nature of the method (makes it static) or access (creates a class instance) it still might not typecheck (e.g. invalid number of parameters). YMMV 🌹

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

6 participants