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

add concept for pluggable program factories #227

Closed

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Mar 9, 2019

Okay, so I've been playing around a bit.

I've added a usage example here:
https://github.com/phryneas/fork-ts-checker-webpack-plugin-docz-test/blob/master/doczrc.js

and the code for @phryneas/fork-ts-checker-plugin-mdx is here:
https://github.com/phryneas/fork-ts-checker-plugin-mdx

As a next step, I would recommend moving the vue code to a separate package as well and maybe remove the vue configuration option.

But that would be a breaking change - the upcoming version increment 1.0.0 could be a good chance for that as it would be a major version step?

: options.useTypescriptIncrementalApi;

/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment says - I would add a warning like this, but it would break current behaviour.

@phryneas
Copy link
Contributor Author

phryneas commented Mar 9, 2019

Hm, it seems that node 8 has a few problems with the default parameter in loadPluggableProgramFactory (?).

I'll wait for your comments first - if you like the general approach, I'll clean that up later :)

@johnnyreilly
Copy link
Member

Wow you've been busy! 10 out of 10 for getting stuck in 😄

I'm afraid I've got a bunch of other things on my plate right now and so I can't give this the attention it deserves right now. Once everything else calms down I'll take a look. In the meantime - thanks for your effort!

@phryneas
Copy link
Contributor Author

No worries, this was kind of fun ^^

Also, I had taken on the assumption that the incremental api would be impossible for vue & co and seeing #220 that won't be like that for long. So maybe my approach of only supporting the basic IncrementalChecker might have been a bit short-sighted.

So I guess waiting for #220 might be a good idea anyway. Or I take some more time and play around with useTypescriptIncrementalApi, too. We'll see :)

@phryneas
Copy link
Contributor Author

phryneas commented Mar 10, 2019

This kept me busy.

I've got another branch with a completely different approach here:
https://github.com/Realytics/fork-ts-checker-webpack-plugin/compare/master...phryneas:typescript-sys-proxy?expand=1

Essentially, in src/service.ts I'm wrapping the typescript.sys object in a Proxy (that object is passed down everywhere). That Proxy takes control over reading, watching, etc. of files. For .ts or .tsx files, it keeps normal behaviour. If it encounters a pre-defined extension (in this example case .mdx), it either appends .__fake__.ts(x) to the file name, so that to the compiler host/program/etc. this file looks like a ts(x) file.

If such a file is read, the content can be processed by a loader. The mdx-specific-code here is 20 lines - and I guess, for vue it wouldn't be longer.

Of course, all that could still be combined with the pluggable approach I took above to keep loader code out of the main repo - but in this case, it could be used for multiple extensions with different loaders at the same time (combining vue with the mdx-pluggable-program from above wouldn't have been possible before).

This works with useTypescriptIncrementalApi: true for me, but currently not with useTypescriptIncrementalApi: false - but fixing that should be easy. In the end, no special code should be required for either one in combination with external loaders - which would be an elegant solution for #220 as well.

Of course, this is very prototype-y and needs some cleanup, but I guess you'll be able to see what I'm going for - so once you find the time, I'd be intrigued by your thoughts :)

PS: also, with a little more work, that should make most of that replicated module resolution code obsolete.

@johnnyreilly
Copy link
Member

Wowser 😁 That does sound like a potentially useful approach. Very interesting!

@stale
Copy link

stale bot commented Jul 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@piotr-oles
Copy link
Collaborator

I will close it as I was able to release v5.0.0-alpha.1 version :)

@piotr-oles piotr-oles closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants