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 regular Node module imports instead of absolute paths in source file #8

Closed
willsteinmetz opened this issue Jul 23, 2019 · 7 comments

Comments

@willsteinmetz
Copy link

While attempting to import the web component as part of the bundle with webpack, it threw a bunch of errors that didn't make sense. I then decided to just reference the source file instead of what was defined in main but that was also a problem.

Currently, the imports in the source file are absolute paths to the specified modules instead of just referencing the modules. Most build tools will handle looking in the node_modules folder if it is it not an absolute path.

ex:

// src/wc-markdown.js
import Prism from '../node_modules/prism-es6/prism.js';
import Showdown from '../node_modules/showdown/dist/showdown.mjs';

should be:

import Prism from 'prism-es6/prism.js';
import Showdown from 'showdown/dist/showdown.mjs';
@willsteinmetz willsteinmetz changed the title Use regular imports instead of absolute paths in source file Use regular Node module imports instead of absolute paths in source file Jul 23, 2019
@evanplaice
Copy link
Member

evanplaice commented Jul 23, 2019

This package targets VanillaJS, not build tools or ESM-ish imports that mimic Node style bare import specifiers (ie no 'js' extension).

In other words, this package works in browsers as-is without needing to be built/bundled/transpiled.

For build tooling support, 'dist' contains a pre-built bundle that already includes the necessary imports.

@evanplaice
Copy link
Member

If you use the following

import wc-markdown from 'wc-markdown';

It should pull in the dist bundle.

@willsteinmetz
Copy link
Author

willsteinmetz commented Jul 23, 2019

Whenever I import the version from dist using the suggestion above, I get all kinds of build errors. I can't put the output from my work here but the one I get doing a barebones app at home is the following (which was also one of the ones I got at work):

ERROR in ./node_modules/@vanillawc/wc-markdown/dist/wc-markdown.js
Module not found: Error: Can't resolve 'jsdom' in '/Users/will/Projects/temp-wc-markdown/node_modules/@vanillawc/wc-markdown/dist'
 @ ./node_modules/@vanillawc/wc-markdown/dist/wc-markdown.js 1331:14-30
 @ ./index.js

That is using Webpack 4 with a config that just takes the entry file and bundles it to an output directory. There are no plugins or other transformations happening.

@evanplaice evanplaice reopened this Jul 23, 2019
@evanplaice
Copy link
Member

Damn, that definitely isn't right.

The Showdown package used here has a lot of legacy junk. This specific error is caused by a conditional require that should only be used during testing. Looks like Webpack is following the require even though it should be conditionally excluded.

https://github.com/evanplaice/showdown-es6/blob/6fed4b97dc91480139cac91dab9ed0ef9fde6d33/dist/showdown.mjs#L579

I'll release a patch fix. Any other notable errors?

@willsteinmetz
Copy link
Author

That was the only one popping up at the moment but I can take a look later and see if there were others that I missed.

I saw your other issue for replacing Showdown with something else. Do you need help looking into other libraries?

@evanplaice
Copy link
Member

evanplaice commented Jul 25, 2019

For now I think replacing Showdown w/ Marked will be a good choice. Showdown is like 100k vs Marked 2k. AFAIK, Marked should be compatible with PrismJS; it just might require a configuration option so the CSS classes the parser attaches to code blocks for syntax highlighting are named properly.

Also, the Showdown fork I use here was supposed to be temporary until the ES module compatibility lands upstream (showdownjs/showdown#661). At this point, I doubt it'll ever land so I'd strongly prefer to kill the fork and replace it with a stable dependency.

In the future I'd like to use a lib written as an ES module for even better savings. I have found a few but none seem to be stable or well-maintained enough yet.

@evanplaice
Copy link
Member

OK, apparently swapping the parsers was a relatively painless process. I added a 'module' field to package.json that points to the dist bundle. Using a regular default import should work w/o issue.

In addition changing parsers cut the prod bundle size down from 193KB to 64KB. Not too shabby.

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

No branches or pull requests

2 participants