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

Updated mongoose timestamp #206

Merged
merged 1 commit into from
Mar 12, 2016
Merged

Updated mongoose timestamp #206

merged 1 commit into from
Mar 12, 2016

Conversation

Fank
Copy link
Member

@Fank Fank commented Mar 12, 2016

I updated the code to use import instead of require
before the only way to import it:

const timestamp = require("mongoose-timestamp");

and now import is also possible

import * as timestamp from "mongoose-timestamp";

Typings URL: https://github.com/Fankserver/npm-mongoose-timestamp
Source URL: https://github.com/drudge/mongoose-timestamp

Was the way i changed the code right?
Is there a other way to use import?

@blakeembrey
Copy link
Member

For CommonJS module.exports = style code, you can import it using import x = require(...) which is probably what you would have used before.

blakeembrey added a commit that referenced this pull request Mar 12, 2016
@blakeembrey blakeembrey merged commit 08c0eea into master Mar 12, 2016
@blakeembrey blakeembrey deleted the fk_mongoose_timestamp branch March 12, 2016 22:03
@unional
Copy link
Member

unional commented Mar 12, 2016

This need to be part of the writing guideline. 😄
It is the only way I see to get around the import interop issue at the moment.
Unfortunately that means typings author needs to know more about these kind of detail.

@Fank
Copy link
Member Author

Fank commented Mar 12, 2016

@blakeembrey no require
-> https://www.npmjs.com/package/tslint "no-require-imports disallows invocation of require() (use ES6-style imports instead)."

@unional
Copy link
Member

unional commented Mar 12, 2016

@Fank , yeah, but that is a hint and "generally" it is fine for application engineer. But it does not work well for typings author.

I'm created an issue on TypeScript about this: microsoft/TypeScript#7398

@Fank
Copy link
Member Author

Fank commented Mar 13, 2016

@unional Thanks for the info, yea writing typings for such a syntax is pain in the ***

@blakeembrey
Copy link
Member

@Fank The problem is there is no ES6-style imports that interop with Node's CommonJS-style module.exports - that's just how the specs work. Even if you avoid it on the linter, you're really just working around the only way to do things. I think it might be better, until TypeScript enables some kind of interop for this CommonJS-style, to make that rule only apply to relative exports that you control. I've seen this confusion a lot now.

@unional
Copy link
Member

unional commented Mar 13, 2016

The problem is there is no ES6-style imports that interop with Node's CommonJS-style module.exports - that's just how the specs work.

Actually that is not completely accurate. That's what the allowSyntheticDefaultImports introduced in TS 1.8 is for. module = systemjs and module = commonjs + allowSyntheticDefaultImports = true interop module.exports to import domready from 'domready'.

The problem remain is import * as domready from 'domready', which is still inconsistent:

  • module = systemjs returns { default: domready }
  • module = commonjs returns domready

@unional
Copy link
Member

unional commented Mar 13, 2016

And, as mentioned in that issue, the critical problem with this interop is that it is not captured in the resulting .d.ts because they are not compiled.

So it is subject to the consumer's configuration and it is bounded to cause grief.

I've seen this confusion a lot now.

Yeah, this was probably the FIRST question I bug Blake. 😜

@blakeembrey
Copy link
Member

@unional It shouldn't be captured in the .d.ts file though as then it would be inaccurate.

On the import * as interop, that's more of an accidental feature to me. It's only possible to do that style of import with TypeScript when it's a namespace, not when it's a plain function (hence it didn't work before this PR). So the only reason it does work is for pre-1.5 interop when then only way to describe multiple exports was with declare module x {}; export = x;. Since all definitions had to be written like this, TS supported it when 1.5 was released with ES6-style imports and exports.

The allowSyntheticDefaultImports was what I was alluding to when I said:

I think it might be better, until TypeScript enables some kind of interop for this CommonJS-style, to make that rule only apply to relative exports that you control.

That said, the definition should always be correct according to the source and ideally we leave fixing these hacks to TypeScript instead of introducing the hacks ourselves which really makes things harder to evolve later.

@unional
Copy link
Member

unional commented Mar 13, 2016

100% agrees that we should leave these hacks to TS

That's why I should include this into the style guide.

About capturing, for the sake of discussion, it capturing the intent.

Say the package author wrote import * as dr from 'domready' with module = systemjs. That means he intent to use it as { default: domready }. This intent needs to be capture and resolve correctly at the consumer project or else it will fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants