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

Fix dependencies based voyager build change #32

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ajainarayanan
Copy link

@ajainarayanan ajainarayanan commented Aug 31, 2017

Based on vega/voyager#678 and vega/voyager#726

  • Modifies package.json to include modules marked as external by voyager.
  • Adds webpack config to include font-awesome and font-awesome-loader to be built as dependencies.
  • Modifies renderer and index.html to require correct modules based on voyager changes.

Note:

@ajainarayanan ajainarayanan changed the title Fix deps based voyager build change Fix dependencies based voyager build change Aug 31, 2017
@@ -0,0 +1,36 @@
const path = require('path');
Copy link
Member

Choose a reason for hiding this comment

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

@ajainarayanan I though you say the normal lib build works fine -- do we really still need this?

Copy link
Author

Choose a reason for hiding this comment

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

This is required for font-awesome-sass-loader Since we marked font-awesome-* as external in voyager we need to add this. This is to parse font awesome font files and serve them using url-loader.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the webpack build step to generate a library build explicitly for electron environment in the voyager PR. This however is required for using font-awesome loader in voyager.

@kanitw
Copy link
Member

kanitw commented Oct 24, 2017

@ssharif6 @FelixCodes -- can you help test if this is still working well?

@ajainarayanan
Copy link
Author

@kanitw Have updated the PR. This PR requires the corresponding PR in vega/voyager-server#4 to be merged before this is merged.

I am sorry for the circle of PRs but unless we release a new version of voyager I don't know how we can merge vega/voyager-server#4 and this PR.

Do let me know if there is an easier way.

Note:

Please don't merge this PR yet as the node modules still point to local npm modules.

@kanitw
Copy link
Member

kanitw commented Dec 4, 2017

@FelixCodes -- you mention you have some comments for this PR the other day. Can you post them so @ajainarayanan can take a look?

I am sorry for the circle of PRs but unless we release a new version of voyager I don't know how we can merge vega/voyager-server#4 and this PR.

Oh you can use yarn link command to help link between local projects. :)

@ajainarayanan
Copy link
Author

@kanitw I understand yarn link but we cannot merge the voyager-server PR before this one as it depends on this version of voyager. And voyager-electron PR cannot be merged without the changes from voyager-server as voyager-electron depends on these changes. Not sure if I am making sense but thats what I mentioned as circle of PRs.

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.

2 participants