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 pre-compiled JavaScript artifacts for Perspective package #378

Closed
wants to merge 25 commits into from

Conversation

LukeSheard
Copy link
Contributor

@LukeSheard LukeSheard commented Jan 12, 2019

This PR does two things to enhance and improve the development workflow for apps running on the webpack plugin provided by @jpmorganchase/perspective:

  1. Split the plugin itself into a separate package - this reduces overhead for installation when end users are only the UMD bundles.
  2. The plugin now uses pre-compiled JS assets (via @babel/cli). This reduces the overhead on webpack for end users since it doesn't need to run babel itself on every build for the package anymore.
  3. Webpack plugin now takes options for the various loaders needed during compilation so an end user can choose where the worker code will be written to (useful for caching).
  4. Perspective Viewer + Plugins now exports a CJS module with it's dependencies externalised.

@LukeSheard
Copy link
Contributor Author

@texodus any ideas why the /pr build failed for a screenshot change? Not sure how this change could affect that one individually.

https://travis-ci.org/jpmorganchase/perspective/jobs/478847521#L870

@timkpaine timkpaine added JS enhancement Feature requests or improvements labels Jan 13, 2019
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

There seem to be some issues with this, notably that the example webpack project no longer compiles. Might I recommend fixing this example, which is both necessary and a good demonstration of what this refactor looks like from a consuming developer's standpoint?

@LukeSheard
Copy link
Contributor Author

LukeSheard commented Jan 19, 2019

@texodus Updated the PR with all the comments we've discussed in person and on here. Still running into this issue when running the webpack example locally though:

screen shot 2019-01-20 at 11 08 13 am

@LukeSheard
Copy link
Contributor Author

To reduce the scope creep of this PR I've raised #390, #391 and #392 and #393 which fix a lot of the initial issues I had with getting the webpack-plugin to work as expected.

To avoid conflicts once those are merged I can raise the final webpack-plugin PR.

@LukeSheard LukeSheard closed this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or improvements JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants