Improve documentation: include usage of ES6 Modules in browser #4099
Conversation
README.md
Outdated
|
|
||
| ```js | ||
| import hljs from './node_modules/@highlightjs/cdn-assets/es/core.js'; | ||
| import javascript from './node_modules/@highlightjs/cdn-assets/es/languages/javascript.min.js'; |
There was a problem hiding this comment.
Isn't this assuming a lot about someone's file structure? I feel like publishing node_modules onto a public site is a bit of a smell, is that normal these days?
There was a problem hiding this comment.
Ah you addressed that below... I'll mull on this.
There was a problem hiding this comment.
Thanks.
First of all, use of web technologies (HTML, CSS, JS) is no longer limited to website/webapp. With the help of Electron, web technologies can used in desktop app as well. This means importing from node_modules is no longer a concern in desktop app.
Now, about the public site, you are right that it is not normal (or lets say, not common in practice) to publish node_modules. I think it is because of lack of awareness about different ways npm packages can be installed in a project. Most of people think that package.json stays at the root directory and all packages should be installed at the root directory. However, we can create separate package.json at public directory for browser specific packages.
/
/package.json (may contain server-specific packages like Express, or may only contain devDependencies like Webpack in case the app is not using Node for server (for e.g. PHP app))
/node_modules/
/public/package.json (contains browser-specific packages like highlight.js, bootstrap)
/public/node_modules/
/public/index.html
This way, packages at top level and any other server specific code does not get exposed to the public.
There is a change of security issue if a npm package contain file that can be directly executed by server (for e.g. PHP file). In that case, a CVE should be created.
Lastly, we can generate different code for the prod environment. Tools like Vite may be helpful regarding that. This way, we can use original code for development and generated code for production.
I can also update the doc to include warning:
Warning: publishing `node_modules` can lead to security problem. Use with caution.
If you think it is not a good idea at all to have node_modules being publicly exposed, I can update the doc to manually copy code from @highlightjs/cdn-assets package and paste in public directory (or create symlink).
Let me know what you think.
README.md
Outdated
| To import the library and register only those languages that you need: | ||
|
|
||
| ```js | ||
| import hljs from './node_modules/@highlightjs/cdn-assets/es/core.js'; |
There was a problem hiding this comment.
What is the ./? Is this assuming a build system or nobuild? Don't real in-browser ES6 imports need to be full URLs?
There was a problem hiding this comment.
You need to check this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#importing_features_into_your_script
Also check below for using bare specifier (import hljs from '@highlightjs/cdn-assets/es/core.js';) using importmap.
Code in @highlightjs/cdn-assets package is browser compatible, so nobuild.
There was a problem hiding this comment.
Hello, is this change somehow related to rails/importmap-rails#270 ?
There was a problem hiding this comment.
I'm familiar with this actually, I was thinking of someone pulling from from a CDN I suppose... I'll look at this and tweak the examples. I'm not going to recommend that people use node_modules though...
I was also thinking of the FIRST import, which is (surely?) going to be relative to the HTML file... relative references for assets are such a pain (IMHO) on a large website. I imagine named imports vs importmap solve a lot of that though.
Taking a look at this more now.
|
Thoughts? |
Build Size ReportChanges to minified artifacts in 1 file changedTotal change -1 B View Changes
|
README.md
Outdated
| ``` | ||
|
|
||
| *Note: The above code works only when the file containing the JavaScript code and the `node_modules` folder are inside same folder. If that is not the case, you have to modify the path in import statement accordingly.* | ||
| *Note: The path to these files will depend on how your individiaul project is configured, |
There was a problem hiding this comment.
In addition to this, I think it is better to be more explicit by including instruction to copy @highlightjs/cdn-assets from node_modules (or CDN) into assets/js/@highlightjs/cdn-assets.
joshgoebel
force-pushed
the
improve-documentation
branch
from
7b2a6de to
b277876
Compare
Build Size ReportChanges to minified artifacts in No changesNo existing files changed. |
Build Size ReportChanges to minified artifacts in 3 files changedTotal change -7 B View Changes
|
3 participants
Changes