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

Export main plugin class and update all imports ? = require(?) to named exports #163

Merged
merged 4 commits into from
Oct 6, 2018

Conversation

milewski
Copy link
Contributor

@milewski milewski commented Oct 4, 2018

this PR allows the types exposed by this plugin to be effectively used inside a .ts file.

the imports can be as:

import { ForkTsCheckerWebpackPlugin } from 'fork-ts-checker-webpack-plugin'
import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'
import * as ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'

currently only this variants are allowed:

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')

this PR also updates all commonjs style imports to named imports/export and closes #162

@johnnyreilly
Copy link
Member

Thanks for the work! Looks good. With this change do the existing imports still work? i.e.

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')

If not then this would need to be a breaking changes release.

Could you also update the CHANGELOG / version number in the package.json please?

@milewski
Copy link
Contributor Author

milewski commented Oct 4, 2018

it is a breaking change... the output of that object would be:

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')

console.log(ForkTsCheckerWebpackPlugin)

{
  default: [Function ForkTsCheckerWebpackPlugin]
  ForkTsCheckerWebpackPlugin: [Function ForkTsCheckerWebpackPlugin]
}

so the import should be like this in this case:

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin').ForkTsCheckerWebpackPlugin
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin').ForkTsCheckerWebpackPlugin

or

const { ForkTsCheckerWebpackPlugin } = require('fork-ts-checker-webpack-plugin')
import { ForkTsCheckerWebpackPlugin } = require('fork-ts-checker-webpack-plugin')

similiar how it was done here: d48440f#diff-f41e9d04a45c83f3b6f6e630f10117feL16

@milewski
Copy link
Contributor Author

milewski commented Oct 4, 2018

but hold on, i think there is a way to keep the exported object like this:

{
  [Function ForkTsCheckerWebpackPlugin]
  default: [Function ForkTsCheckerWebpackPlugin]
  ForkTsCheckerWebpackPlugin: [Function ForkTsCheckerWebpackPlugin]
}

so it wouldn't be a breaking change let me do some experiments here first..

@johnnyreilly
Copy link
Member

Sure

@milewski
Copy link
Contributor Author

milewski commented Oct 4, 2018

OK i got it working, but this has to assume that user tsconfig.json module is set to commonjs if it set to esnext for example this syntax wont work

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')

but this will

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin').ForkTsCheckerWebpackPlugin
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin').ForkTsCheckerWebpackPlugin

or

const { ForkTsCheckerWebpackPlugin } = require('fork-ts-checker-webpack-plugin')
import { ForkTsCheckerWebpackPlugin } = require('fork-ts-checker-webpack-plugin')

or

import { ForkTsCheckerWebpackPlugin } from 'fork-ts-checker-webpack-plugin'
import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'
import * as ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 4, 2018

So this commit is very noisy; could you undo the whitespace changes please? I can't make out what's actually changed here... 2106654

Probably we should switch to using prettier for consistent formatting in future.

It's unclear to me whether consumption is still unchanged; I'd like users to be able to use this consumption ideally:

const  ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')
import ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin')

It feels that ought to be possible. ts-loader manages it. Maybe this helps?

https://github.com/TypeStrong/ts-loader/blob/d28ba6f732346067535d25889b8d56c95961c4a6/src/index.ts#L505

https://github.com/TypeStrong/ts-loader/blob/master/src/tsconfig.json

@milewski
Copy link
Contributor Author

milewski commented Oct 5, 2018

@johnnyreilly sorry my IDE had that format before commit option on..... forgot to unckeck.. it is clean now...

and if i only export it like this:

export = ForkTsCheckerWebpackPlugin

it would be exactly the same as it was originally.... so i would still face the same problem importing it..

and what i meant about this being a breaking change or not... let says that the user has module: commonjs on his tsconfig.json, this wouldn't be a breaking change for them, however those with the option set to module: esnext or es2015 would break, because it would no longer be a commonjs import but ES one.. which has to be imported as any of these forms:

import { ForkTsCheckerWebpackPlugin } from 'fork-ts-checker-webpack-plugin'
import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'
import * as ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'

and of course importing by require('fork-ts-checker-webpack-plugin') would still work the only difference is that doesnt return a function but an object like this:

{
  default: [Function ForkTsCheckerWebpackPlugin]
  ForkTsCheckerWebpackPlugin: [Function ForkTsCheckerWebpackPlugin]
}

so i think this change is a breaking change, but it would only affect a very low percentage of this plugin users... because their webpack.config would have to be written in .ts for them to feel any effect, which i believe everyone defaults to a .js file..

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 5, 2018

Hey @milewski

Thanks for that. I want to clarify the situation for users writing vanilla js webpack.config.js files (most users)

They can still write const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin') and they don't need to then .ForkTsCheckerWebpackPlugin to use the plugin? i.e. it's a default export still?

@milewski
Copy link
Contributor Author

milewski commented Oct 6, 2018

@johnnyreilly sorry for the confusion... i followed the example u sent (https://github.com/TypeStrong/ts-loader/blob/d28ba6f732346067535d25889b8d56c95961c4a6/src/index.ts#L505) and this seems to be a much better solution, by doing so i dont need to change the export method and it will work for .js and .ts with the exacly same signature

the last commit reverts the export back how it was originally and

for .js

console.log(require('fork-ts-checker-webpack-plugin'))
// { [Function: ForkTsCheckerWebpackPlugin] }

for .ts

console.log(require('fork-ts-checker-webpack-plugin'))
// { [Function: ForkTsCheckerWebpackPlugin] }

import * as ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'
console.log(ForkTsCheckerWebpackPlugin)
// { [Function: ForkTsCheckerWebpackPlugin] }

// Invalid imports for .ts, which is ok as there is at least 1 that works for .ts files
import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'
import { ForkTsCheckerWebpackPlugin } from 'fork-ts-checker-webpack-plugin'

so this PR is not a breaking change for neither .ts users nor .js users

@johnnyreilly
Copy link
Member

Great work! Do you want to update the package.json to a new version please? And also update the CHANGELOG.md too. That makes it easy for us to ship a release

@milewski
Copy link
Contributor Author

milewski commented Oct 6, 2018

done 👍

@johnnyreilly johnnyreilly merged commit 237b1fd into TypeStrong:master Oct 6, 2018
@johnnyreilly
Copy link
Member

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.

Unable to compile typescript after importing the latest version to my config files...
2 participants