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

[WIP] Quotation support #1839

Open
wants to merge 46 commits into
base: nagareyama
Choose a base branch
from
Open

[WIP] Quotation support #1839

wants to merge 46 commits into from

Conversation

alfonsogarciacaro
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro commented May 20, 2019

Continuation of #1818. Credit goes to @krauthaufen. Opening the PR to check the diffing with master while we evaluate. The branch has been published as fable-compiler-quotations to npm, can be tested by installing the package and then adding "compiler" to your Fable options like:

You also need to define the FABLE_QUOTATIONS constant to activate quotation support.

            {
                test: /\.fs(x|proj)?$/,
                use: {
                    loader: "fable-loader",
                    options: {
                        babel: CONFIG.babel,
                        define: "[FABLE_QUOTATIONS"],
                        compiler: "fable-compiler-quotations",
                    }
                },
            },

# Conflicts:
#	src/Fable.Transforms/FSharp2Fable.fs
#	src/Fable.Transforms/Fable2Babel.fs
#	src/fable-library/Reflection.ts
@et1975
Copy link
Member

et1975 commented Nov 2, 2019

Would love to see this finally land.

@alfonsogarciacaro
Copy link
Member Author

Yes, my last thinking was to turn this into a next branch and start releasing Fable 3 alpha versions with it. However, I've just started a new professional project and have very little time available for open source now, so I'm afraid I won't be able to meet the time and energy releasing a new major version requires.

It'd be great if someone would step up and lead the next major release. I would still be providing support, of course. Any volunteers? :)

@krauthaufen
Copy link
Collaborator

krauthaufen commented Nov 5, 2019

Hey, I'm not really capable of doing that, but I can certainly finalize the quotation/reflection things.
I'm currently working on Fable.Elmish.Adaptive but once this is done I'll come back to this.
Just let me know if I can help.

@gitpod-io
Copy link

gitpod-io bot commented Apr 27, 2020

@ncave
Copy link
Collaborator

ncave commented Jun 4, 2020

@alfonsogarciacaro See #2075.

@krauthaufen
Copy link
Collaborator

Hey there,
I've been quite busy with other projects recently, but if there's interest in bringing that up to speed I'd totally be willing to help. Just let me know if you're still interested in integrating that...

@alfonsogarciacaro
Copy link
Member Author

That'd be great @krauthaufen! I'm about to write an issue for the roadmap for Fable 3, so we can continue the discussion there. One quick question though, all the reflection support added in this PR is necessary for supporting the quotations or just to be able to execute a quotation in runtime?

@krauthaufen
Copy link
Collaborator

krauthaufen commented Jun 9, 2020

Hey, we could certainly support quotations without MethodInfo/PropertyInfo invokes but in many cases these are required...
Maybe introduce a compiler-flag or something?

I'm not sure regarding the new reflection, but for quotations reflection needs to be more or less feature-complete since they need basically everything (ConstructorInfo, MethodInfo/etc.)

@Shmew
Copy link
Contributor

Shmew commented Jun 9, 2020

It would be great if there was a way to tree-shake type data that isn't used. Then this becomes a non-issue

This was referenced Jun 12, 2020
@alfonsogarciacaro alfonsogarciacaro changed the base branch from master to nagareyama December 3, 2020 06:25
@chkn
Copy link
Contributor

chkn commented Mar 21, 2021

Hello, I have a project that needs code quotations. What is the status of this? Is there anything I could do to help?

One idea I'm investigating is to create a separate Fable.Reflection NuGet that uses a plugin to add reflection data. Would that be a better approach if people are worried about adding extra data?

@alfonsogarciacaro
Copy link
Member Author

I was planning to merge this with Fable 3, but then focused on other goals and unfortunately this branch is diverging more and more from the main one (nagareyama atm). Having a specialized Fable.Reflection package is a good idea, although I'm not sure if it can work with quotations, because these rely on the .net base reflection API if I'm not mistaken.

The first step would be to sync this with nagareyama again. My main worry is that iirc this branch increases reflection support but adds a direct reference to all class methods in the reflection info which will prevent tree shaking and increase the bundle size. A solution could be to only add methods to reflection info when a compiler constant is declared as it's already done with FABLE_QUOTATIONS.

@chkn
Copy link
Contributor

chkn commented Apr 8, 2021

Having a specialized Fable.Reflection package is a good idea, although I'm not sure if it can work with quotations, because these rely on the .net base reflection API if I'm not mistaken.

The idea was to have a Fable.Reflection package implement the base .NET reflection API, and quotation support could be included in that or in another package that depends on it. Of course, having it "in the box" would also be good.. I'm just trying to understand the concerns.

My main worry is that iirc this branch increases reflection support but adds a direct reference to all class methods in the reflection info which will prevent tree shaking and increase the bundle size.

Hmm maybe we should differentiate the level of reflection support needed for code quotations from full support. Full reflection by its nature is not really amenable to tree shaking because someone could reflect any member of any type dynamically, so we must include all data. Code quotations on the other hand I think are fairly static expressions, where we know at compile time what reflection data is needed. In that case, perhaps they could just reference the specific reflection data and the rest would be omitted?

@alfonsogarciacaro
Copy link
Member Author

Sorry for the late reply. I might be entirely wrong because it's been a long time since I haven't checked this branch, but I think the PR includes reflection support for invoking methods in order to run the quotations. Maybe it could be a good compromise to implement the PR without this for when there's no need to run the quotations, although unfortunately I'm not sure how much work that would entail.

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.

9 participants