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

use npm highlight.js instead of vendoring #16

Closed
wants to merge 1 commit into from
Closed

Conversation

d4l3k
Copy link

@d4l3k d4l3k commented Aug 27, 2018

This fixes #15, #9

@kcmr
Copy link
Owner

kcmr commented Aug 30, 2018

Reviewing next weekend. Thank you.

@kcmr
Copy link
Owner

kcmr commented Sep 23, 2018

Hi @d4l3k

After reviewing the changes I've found the following issues:

  • First of all, the file to edit is inside the src folder. When you launch the component's demo using npm start, it is copied with styles in the project's root. I have to document this.
  • This change requires to use Browserify or another library to use require() in browser. I want to keep the component as standalone as possible.
  • Finally, this change restores the full highlight.js download (more than 100kb). Recently I did a change to prevent this: Back to polymer element #14

I appreciate your contribution but I cannot approve it and merge for now. Maybe you can find another approach to make it work with Browserify that does not require to change the hightlight import inside code-sample.

Thank you.

@d4l3k
Copy link
Author

d4l3k commented Sep 23, 2018

You could provide two builds one with highlight.js compiled in and a second with highlight.js as the import. That way you'd be able to have both benefits with the same code base.

I believe most polymer3 apps use webpack anyways as that's the trend for JS frameworks. Not supporting the recommended way seems like the wrong approach.

Anyways, #17 makes this library a no-go for me so I switched to using the react highlight.js element and embedded it in my polymer3 app.

@kcmr
Copy link
Owner

kcmr commented Sep 23, 2018

Ok, then I close the issue. I think that I can't do anything to fix the #17.
I'll investigate the two builds option.

@kcmr kcmr closed this Sep 23, 2018
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.

import in webpack not working
2 participants