Skip to content

Emit declaration files #71

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

Closed
wants to merge 2 commits into from
Closed

Emit declaration files #71

wants to merge 2 commits into from

Conversation

orkisz
Copy link

@orkisz orkisz commented Sep 17, 2016

Hi @langley-agm,

This is second attempt to help resolve issue with build tools like Webpack going crazy when referencing this package (#39 and #65). My previous PR #70 did not in fact solve problem that Webpack should not compile files that are already included in dist folder. Here comes my solution:

  1. Let's put ES5 node-compliant files into dist/es5 folder
  2. Allow TS compiler to emit declarations both for ES5 and ES6 formats
  3. Let's make ES5 output default since any bundler is going to like this moduie format. That way, you can reference logger by just typing:
    import { Logger } from 'angular2-logger';
    no matter if you write in ES6 or TS.

Of course, referencing by import { Logger } from 'angular2-logger/core'; will still works for users that used it before.

@langley-agm
Copy link
Contributor

Question @orkisz ,

Would it make a difference if we replace all your changes for only this one:

typings": "core.d.ts ?

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

I'm afraid no. As long as main file points out to core.js in root folder, Webpack is going to compile TSes (remember, core.js imports those!).

@langley-agm
Copy link
Contributor

core.js imports a name without an extension, you decide what extension to look for, doesn't have to be .ts

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

One another pros of my solution is defaulting to ES5 build. Please take into consideration that ES5 commonJS module is most widely used format of NPM modules and it is unlikely to cause any troubles in wide range of bundlers.

@langley-agm
Copy link
Contributor

langley-agm commented Sep 17, 2016

isn't the current core.js in the root es5? Making it the default?

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

Yes, it is. The problem is when using imports without file extension, Webpack loaders will use TS files.

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

Let me try approach with just typings statement in package.json...

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

@langley-agm Confirmed: just adding "typings": "core.d.ts" did not help. Webpack still tries to compile TS

@langley-agm
Copy link
Contributor

The problem is when using imports without file extension, Webpack loaders will use TS files.

Supposing it does this by default which I doubt, to me its angular cli configuring it that way, can't you change this configuration?

@langley-agm
Copy link
Contributor

Confirmed: just adding "typings": "core.d.ts" did not help. Webpack still tries to compile TS

We already know this. What I will look for is how to change the configuration so it does not do this.

@langley-agm
Copy link
Contributor

Similar to how the angular team uses SystemJS:

rxjs: { defaultExtension: 'js' },

They specify that whenever rxjs is requested, the loader will use .js for the extension.

Remember webpack is merely a bundler, it does not compile. Angular cli is the link between your code, webpack and typescript configuring it in a certain way.

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

I don't use angular-cli, but plain Webpack. Maybe I can reconfigure it to take js files before ts, but can you ensure me that it won't cause other modules to fail?

I can't get it. There is something obviously wrong with your build process:

  1. Destination files go to same directory as source (app) - bad practice
  2. Import process is different depending on end-user bundler configuration
    but after many pages of conversation (Type 'string' is not assignable to type 'Level' #39 and ERROR in [default] \node_modules\angular2-logger\app\core\logger.ts:62:38 Type 'string' is not assignable to type 'Level'. #65) you still deny to admit it. Can you at least tell me what is wrong in approach proposed by me in this PR?

@langley-agm
Copy link
Contributor

langley-agm commented Sep 17, 2016

Maybe I can reconfigure it to take js files before ts, but can you ensure me that it won't cause other modules to fail?

You configure this module, not a global configuration, just like the example I showed you configures only rxjs.

but after many pages of conversation (#39 and #65) you still deny to admit it

I haven't said there isn't anything wrong with the current build process once. What I've said a million of times is that you should not compile third party libraries and I want to find the root cause of this issue that is making your project do so. Not a workaround patch. I haven't said any of these PRs is wrong, there's a reason they are still open after all. What I'm trying to do is to get clear on the assumptions we're making. As far as I understand Webpack has no idea of Typescript and as such it cannot compile them, I can be wrong, that's why I said I'll search into it.

  1. Destination files go to same directory as source (app) - bad practice

Do you have any official source of this? Someone actually saying its bad practice? These technologies are far too new to decide over best practices yet imho, even the Angular team keeps changing them in a matter of months, not so long ago they had them the way I do. I'm not saying they're correct the way they are but lets not make wrong assumptions just for the sake of doing them.

  1. Import process is different depending on end-user bundler configuration

This is definitely not a goal, and to correctly fix it I need to find the root cause of the problem. Note that the current importing process is the same as in the Quickstart Angular page, trying to look for something similar with Webpack is the opposite of a different process, is actually looking a way to do it the same way.

I don't use angular-cli, but plain Webpack.

This is interesting could you share an example? Are you not using any third parties that augment webpack?

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

You configure this module, not a global configuration, just like the example I showed you configures only rxjs.

I believe it's not possible in Webpack...

As far as I understand Webpack has no idea of Typescript and as such it cannot compile them, I can be wrong, that's why I said I'll search into it.

Ok, I'm going to comment it in the end of this message.

Do you have any official source of this? Someone actually saying its bad practice?

I don't think there is any official source. Just my practice, few samples:

  1. Cleaning before build - just rimraf dist,
  2. .gitignore have dist folder and .npmignore have src
    But OK, maybe I went too far considering my opinions as standard

Let me know you why it's so important to me. I like your logger, I really do. For me, there is no equivalent available yet. But I must go live with my app migrated to ng2 final (TS compiler version 2...) before Monday EOD and this is only module causing build to fail. I can either do a) fork and publish my version of module (what I don't want do to since it's your work and additionally I don't want to loose easy-to-get updates from you) b) use other logger (but time window is tight to do so) c) completely get rid of logging facility.

Please at least tell me if there is any chance you will consider changes in build process until Monday. If no, I'm going to choose a, b or c. There is simply no time for discussion for me if it is pointless.

@orkisz
Copy link
Author

orkisz commented Sep 17, 2016

I actually have an idea for a very quick-fix: let's just add .ts files to .npmignore. This will remove a root cause of the problem.

As a follow up to previous message: I think an action is required since more and more people will migrate to ng2 final next week and they will experience the same issue.

@langley-agm
Copy link
Contributor

@orkisz

Please at least tell me if there is any chance you will consider changes in build process until Monday

I will not consider any changes until I find the root cause of the issue. Most likely it won't be by Monday.

My suggestions:

Option 1- Try the officially released version of Typescript 1.8 instead of 2+, which should not give you this error. If Angular doesn't work with it anymore try option 2.

Option 2- Point to either your fork or deity's until next week when I fix this specific error. This is a very powerful feature of git and npm, no reason to disregard it =).

A note of advice: I will try to keep it as import { Logger } from 'angular2-logger/core.

@langley-agm
Copy link
Contributor

I actually have an idea for a very quick-fix: let's just add .ts files to .npmignore. This will remove a root cause of the problem.

It will just hide it, there is a value on having those files in the distribution, I won't remove them.

@orkisz
Copy link
Author

orkisz commented Sep 18, 2016

Ok, I will follow option 2. then. But let me try to point you out what in my opinion causes problems.

Let's look on your app's source structure:

core.ts
app
     |
     core
           |
           ---logger.ts
           ... and other files

core.ts contains:

export * from "./app/core/level";

That how it is, right? So now we compile sources and we get:

core.ts
core.d.ts
core.js
app
     |
     core
           |
           logger.ts
           logger.d.ts
           logger.js
           ... and other files

So far so good, right? So let's suppose we're lazy and forgot to clean all dist files before next compilation.
Now we invoke tsc again and what it does it basically takes core.ts and import all depencencies. But wait, now we have both logger.ts and logger.js... Do we care? No, because tsc compiles .ts files, not .js so they do not matter at all!
If it was opposite, the source code would never be compiled more than once since tsc would always take precompiled JSes as source of truth. Do you agree?
Now, we get back to my Webpack process. The loader for TypeScript is just a wrapper around tsc so it behaves identically. It does not matter for it that npm package exposes js file as entry point; it's main responsibilty is to compile TypeScripts. If export statement has references both to TS and JS it will always take TS. Still logical?
Now, basically, you ask me and other people having same problem to override this very logical process just for your package. Does it make sense?

And just to ask - what is the reason to keep sources in NPM? (just asking!)

@orkisz orkisz closed this Sep 18, 2016
@orkisz orkisz deleted the emit-declaration-files branch September 18, 2016 17:13
@langley-agm
Copy link
Contributor

@orkisz

So far so good, right?

There's no core.ts, there's only core.d.ts, that's why you should not go find the ts files because the .d.ts file already has everything that typescript needs.

The loader for TypeScript is just a wrapper around tsc so it behaves identically. It does not matter for it that npm package exposes js file as entry point; it's main responsibilty is to compile TypeScripts.

Wrong, once again the loader does not compile, you can add this functionality through plugins that will handle tsc, but that's nowhere near its main responsability, its main responsability is to load files, in this case it should load js files.

Now, basically, you ask me and other people having same problem to override this very logical process just for your package.

Never did such a thing.

For reference I just followed both new Quickstart Guides in Angular 2's Web Page and none of them gave this issue even though its using typescript 2+.

https://angular.io/docs/ts/latest/quickstart.html
https://angular.io/docs/ts/latest/guide/webpack.html

So its definitely an issue with your configuration.

I wonder if this part has something to do with it (from webpack.common.js):

resolve: {
    extensions: ['', '.js', '.ts']
},

Does it prioritize adding js before ts?

I only followed their webpack guide, installed the angular2-logger and that's it, I didn't have to configure anything at all.

Now that I've seen its behaving just fine without changes even when using webpack I'm going to fix the
"Type 'string' is not assignable to type 'Level'." error. But I highly suggest that If you were getting this error you take a look at your webpack configuration because compiling third party libraries will make your build proccess ineficient to say the least.

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.

2 participants