search

projectblacklight / blacklight

Blacklight provides a discovery interface for any Solr (http://lucene.apache.org/solr) index.
http://projectblacklight.org/
Other
757 stars 255 forks source link

Fix allowing imports of individual source files/modules from blacklight-frontend npm package #3128

Closed jrochkind closed 5 months ago

jrochkind commented 5 months ago

Closes #3050

All imports should be relative, so individual files can still be imported from blacklight-frontend npm packagerted from blacklight-frontend npm package

It turns out the solution to #3050 was as simple as this! Thank you for letting me rubber-duck it at the blacklight committers meeting, @jcoyne, @tpendragon @hackartisan @tampakis

I have built the NPM package locally, and verified this appears to work in WIP BL8 upgrade, to let me selective import like I did before (with the somewhat odd source path that remians the same from BL7):

import 'blacklight-frontend/app/javascript/blacklight/core';

Or instead you can now also do like (that i don't think you could in BL7 but now can):

import Blacklight from 'blacklight-frontend/app/javascript/blacklight/core'
import Modal from 'blacklight-frontend/app/javascript/blacklight/modal';

I am not an expert in JS or in what BL is trying to do with JS; I don't think this will cause any problems for existing uses, but perhaps @jcoyne or @cbeer want to review?

Not sure how to encourage/instruct people to keep using only relative paths in these source files, since tests don't currently test for anything relevant here, but better than nothing.

jcoyne commented 5 months ago

@jrochkind is the .js suffix a necessary part of this change or can it work without that?

jrochkind commented 5 months ago

@jcoyne While it worked for me without that, some documentation i found suggested that it was necessary in ES6 for relative-path imports. So I added it.

I can't find the exact page I was looking at that said espeically for relative paths, but googling now I see:

Certain bundlers may permit importing files without extensions; check your environment.

—https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

It may depend on exactly what JS bundler you are using? It seemed to do no harm and be extra safe to me, in this wild JS landscape, but it works for me without it so I don't insist.

Thanks for reviewing!

jrochkind commented 5 months ago

Here's another suggestion:

The Node spec requires that you use .js file extensions for all imports and exports. This was decided so that a relative import path like ./foo.js would work both in Node and the browser.

— https://www.totaltypescript.com/relative-import-paths-need-explicit-file-extensions-in-ecmascript-imports

I'm not totally sure if we expect these individual uncombined source files might be used in the browser? That is not my use case.

But from my reserach (not a JS expert!), it seemed like including extension was safer across the diversity of possible bundlers and environments, and did no harm. I definitely defer to anyone who knows more than me though!

jcoyne commented 5 months ago

Thanks for checking that.