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

Update to v0.14.0-rc3 #7

Merged
merged 9 commits into from
Dec 12, 2020
Merged

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez
Copy link
Contributor Author

I'm pretty sure this is blocked by web-dom-parser

@thomashoneyman
Copy link
Contributor

This is failing because of https://github.com/LiamGoodacre/purescript-naturals

@JordanMartinez
Copy link
Contributor Author

@hdgarrood
Copy link

I think we should try to avoid depending on libraries which are outside of core, web, and possibly also contrib in this org. I think we should consider replacing the uses of Natural with Int.

@thomashoneyman
Copy link
Contributor

I don’t mind switching to Int.

@bbarker
Copy link
Collaborator

bbarker commented Dec 12, 2020 via email

@thomashoneyman
Copy link
Contributor

thomashoneyman commented Dec 12, 2020

I think it's sensible for the natural library to move into core, but that's really a decision for @LiamGoodacre to make. If it does move, then we can keep the dependency here.

@hdgarrood
Copy link

I would prefer that we not move natural into core while it's a wrapper around Int. I think a natural number type should ideally be backed by BigInt.

@bbarker
Copy link
Collaborator

bbarker commented Dec 12, 2020 via email

@hdgarrood
Copy link

Right, the other reason I'm not particularly keen on Natural is that I think that in most cases the extra safety it gives you is difficult to take advantage of with the type system we have. With Idris having dependent types, I think having Peano numbers built in makes a whole lot more sense, so that you can have good ergonomics for operations like a vector append function for vectors whose length is tracked in the types. You can technically do this in PureScript too but we generally don't; the ergonomics are horrible and you often have to tell the type system "trust me, this is fine" in cases where you wouldn't have to do that in Idris.

bower.json Outdated Show resolved Hide resolved
@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Dec 12, 2020

Looks like CI still fails because of the non-Type kind annotation I put on ReaderT. Thoughts?

@natefaubion
Copy link

I don’t think that’s why it’s failing. It’s failing to unify Aff with ReaderT. A missing lift?

@JordanMartinez
Copy link
Contributor Author

Nope. It was due to hard-coded signatures elsewhere...

@JordanMartinez
Copy link
Contributor Author

CI now fails with this:

ReferenceError: XPathResult is not defined
    at Object.<anonymous> (/home/runner/work/purescript-web-dom-xpath/purescript-web-dom-xpath/output/Web.DOM.Document.XPath.ResultType/foreign.js:3:20)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/runner/work/purescript-web-dom-xpath/purescript-web-dom-xpath/output/Web.DOM.Document.XPath.ResultType/index.js:3:16)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
* ERROR: Subcommand terminated with exit code 1
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @ test: `eslint src && pulp test -- --censor-lib && node test/node.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @ test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@JordanMartinez
Copy link
Contributor Author

Issue seems due to jsdom?

test/node.js file content:

const { JSDOM } = require("jsdom");
const { XPathResult, DOMParser } = new JSDOM().window;

Object.assign(global, { XPathResult, DOMParser })

require("../output/Test.Main/index.js").main({ browser: false })();

@thomashoneyman
Copy link
Contributor

thomashoneyman commented Dec 12, 2020

Hm. @kl0tl introduced jsdom to fix this specific error:
#6 (comment)

@JordanMartinez
Copy link
Contributor Author

Just FYI. Originally, the tests were never run during CI

@kl0tl
Copy link
Contributor

kl0tl commented Dec 12, 2020

I pushed some commits to address the issue. We cannot run tests with pulp like we usually do because we have to emulate XPathResult and DOMParser with jsdom first, and configure the browser flag to disable the tests that fail with the emulated DOM provided by jsdom.

@bbarker bbarker merged commit 88c73c1 into purescript-web:master Dec 12, 2020
@bbarker
Copy link
Collaborator

bbarker commented Dec 12, 2020

Thanks @kl0tl and everyone.

@JordanMartinez JordanMartinez deleted the updateTo14 branch December 12, 2020 16:34
@@ -29,7 +28,7 @@ import Web.DOM.Element (Element, fromNode, getAttribute)
import Web.DOM.Node (Node, nodeName)

unsafeFromRight :: forall l r. Either l r -> r
unsafeFromRight = either (unsafeCrashWith "Value was not Left") identity
unsafeFromRight = fromRight' (\_ -> unsafeCrashWith "Value was not Right")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JordanMartinez hmm, good catch - if you're referring to the "Left" in the message, I think that was probably a typo on my part, but it should have read "Value was not RIght", as it does now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbarker Not quite. "Value was not Left" would only appear if the value was Left, so it doesn't make sense. "Value was not Right" would only appear if the value was Left, not right. Thus, this message is more accurate. I was saying 'whoops' in regards to what I originally wrote here because it's not correct.

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.

None yet

6 participants