-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Upgraded to Jint v3 #89
Upgraded to Jint v3 #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is superb - you are officially a rockstar 🚀 !
What are you plans with this? How should we proceed to bring this live?
@@ -22,7 +22,7 @@ | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="AngleSharp" Version="0.15.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big question is - should we make this AngleSharp 1.0.0 compatible?
We can also first release it for an older version and then upgrade it (making it also a 1.0.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to target AngleSharp 1.0.0 and then make this a v1.0.0 as well.
I'll make those changes and update my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about target frameworks? I notice that for AngleSharp v1 you target netstandard2.0;net461;net472;net6.0;net7.0;net8.0
.
Should we aim for the same on this project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aim yes, but generally libs don't need to have the same scope, e.g., AngleSharp.Io
has a few things that require "more" capabilities - so it is not as general.
Appreciated that you take the update!
Also another question - what about #87 ? Should we bring this in (the test and fix, if that still applies)? |
I think #87 should be merged in |
Merged #87 into |
9eec319
to
7634f87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome to me. Let's have the build green and have the first pre-release done.
We're note quite there yet. |
Yes, I mean it's anyway just a first preview. I've assigned some / relevant issues to the v1 milestone. I am not sure yet how much I can look into them tonight and on the weekend, but my goal would be to have a bit of focus now on AngleSharp.Js to have this as a 1.0.0. Thanks for your PR and motivating me to do so. If you want to pick up / anyway have some of these issues handled then please comment on them. Otherwise I'll just pick up whatever is left. |
Preview is out (https://www.nuget.org/packages/AngleSharp.Js/0.15.0-beta.16). The version is still placed at 0.15 and I also noticed that some parts of the NuGet package definition (e.g., the README) are missing: I'll add those and push to |
Awesome work @tomvanenckevort ! I'll hopefully have some timesoon too to check this out and investigate if there's something that Jint could improve too. Just have to get back from vacation first 😄 |
Preview is out (https://www.nuget.org/packages/AngleSharp.Js/1.0.0-beta.20); incl. latest target frameworks, correct reference to Jint and AngleSharp, README etc. I think from the infrastructure POV we are done, now we "only" need to clear the outstanding issues and have it (draw the rest of the owl). |
Types of Changes
Prerequisites
Please make sure you can check the following two boxes:
Contribution Type
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Description
As discussed in #84 this PR is to cover the upgrade of AngleSharp.Js to use Jint v3.
Changes made:
FunctionInstance
>Function
) to match the changes made in Jint v3.EngineInstance
to remove the lexical and variable environments since those are no longer public in Jint v3. Instead there is now a globalwindow
object which also gets exposed on the global object (to ensure that things likewindow.location.href
andlocation.href
resolve to the same thing).ConsoleInstance
with a newConsole
class that ties it directly to the Window object (as per JS spec).DomDelegateInstance
>ClrFunction
).Also a shoutout to @lahma for providing some guidance and older test code that helped me figure out the global object changes. 👍