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 "browser" property to package.json #847

Merged
merged 4 commits into from
Feb 11, 2018
Merged

Conversation

intelliot
Copy link
Collaborator

@intelliot intelliot commented Feb 7, 2018

This should be an alternative to #840

We need to run the build script for this file to exist, so how should we handle that?

Perhaps we can just make sure "./build/ripple-latest.js" is prebuilt and published to npm?

See also: #844 (comment)

Need to run `build` script for this file to exist
@intelliot
Copy link
Collaborator Author

@mikeb1rd Could you verify that this solution works for you?

@intelliot
Copy link
Collaborator Author

Note that you'll need to go inside ripple-lib/ and run yarn build (or npm build), otherwise you'll get

Error: Cannot find module 'ripple-lib' from ...

@FredKSchott
Copy link
Contributor

FredKSchott commented Feb 7, 2018

@intelliot that's right, you would build these files on every publish to npm. "prepublish": "npm run clean && npm run compile" should take care of this for us already.

Edit: Oh wait, no, npm run compile doesn't build for web. We should add npm run build to the prepublish script as well.

@intelliot
Copy link
Collaborator Author

Done in 9a789ce. I also bumped the engine version to ">=6.12.3" since that's the version I use and I don't think anyone regularly tests on earlier versions of node.

@intelliot
Copy link
Collaborator Author

intelliot commented Feb 7, 2018

Looks like this actually does not work...
1. lodash isn't included, so I have to add that separately
2. RippleAPI isn't exported(?), so I'm getting TypeError: RippleAPI is not a constructor

I'm not sure what the right approach is. I might need to dig into how browserify is meant to work, unless y'all have other ideas?

@intelliot
Copy link
Collaborator Author

intelliot commented Feb 8, 2018

Okay, I have a working solution now. With bbfc53e, instead of using the webpack build, we are just substituting wswrapper in place of ws (inspired by websockets/ws#1093 (comment)).

This worked for me; @mikeb1rd could you give it a try?

package.json Outdated
@@ -64,7 +67,7 @@
"clean": "rm -rf dist/npm",
"compile": "mkdir -p dist/npm/common && cp -r src/common/schemas dist/npm/common/ && tsc",
"watch": "tsc -w",
"prepublish": "npm run clean && npm run compile",
"prepublish": "npm run clean && npm run compile && npm run build",

Choose a reason for hiding this comment

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

This can be reverted now, as the built files are not used

@mikeb1rd
Copy link

@intelliot Thanks for spending time on this. I've just added one comment, otherwise the solution works well.

@intelliot intelliot merged commit 4f60fc3 into develop Feb 11, 2018
@intelliot intelliot deleted the browser-in-package-json branch February 11, 2018 16:52
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.

3 participants