projectblacklight / blacklight

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

unify importmap and npm use on 'blacklight-frontend', modernize package.json with exports #3371

Closed jrochkind closed 1 month ago

jrochkind commented 1 month ago

For local engine resources used by importmap-rails, we change the name they are pinned as to blacklight-frontend instead of blacklight. No other changes to importmap-rails use case.

This change, to match the npm package name, means that npm-package-users can use the source files in app/javascript/blacklight instead of requiring a separate rollup output. This has a variety of advantages to npm-package-users, including: in the future eliminating the need for rollup; allowing npm-package-users to import single source files (they couldn't before); allowing npm-package-users to use local file system yarn adds, or git yarn adds (they couldn't before, because of need to run prepare script to get rollup output that is not checked into repo).

We also modernize the blacklight-frontend npm package.json to use modern exports instead of deprecated main and module to specify exports, pointing at the common index.js JS source (and making other stuff avail at nice paths).

Paves the way to -- after a future removal of sprockets support -- removing our rollup prepare step entirely.

changes

importmap users (minor backwards incompat, have to change name of import to match new pin name)

Have to change any import Blacklight from 'blacklight[/*]' to import Blacklight from 'blacklight-frontend[/*]'. Easy enough.

(npm is still not required, importmap-rails pins of local engine files are still being used, only change is a different pin name)

blacklight-frontend npm package users

(ie jsbundling-rails, vite)

import Blacklight from 'blacklight-frontend' remains, but now will import the common index.js file instead of the rollup-built file.

You can also now import indiviudal files individually if you want (was impossible before) eg import BlacklightCore from 'blacklight-frontend/core'.

If you WANT the rollup build file (I don't know why anyone would, but it was what they were getting before, and it's there, so made avail), you can import Blacklight from 'blacklight/blacklight.esm.js'

Any JS builders using CommonJS instead of ESM can require('blacklight'); to get the rollup-built non-ESM app/assets/javascripts/blacklight.js file. I don't know if anyone is doing this or why they'd want to, but this was how it worked before this PR, so I preserved it in package.json exports.

Stylesheets

Most sass traditionally have been putting node_modules on the sass load path in order to import npm package scss, which is what rails generated --css=bootstrap does.

So for them, package.json exports aren't used, and import remains the same: @import "blacklight-frontend/app/assets/stylesheets/blacklight/blacklight";

If someone is instead using the new sass npm pkg importer which actually knows about npm packages, they could instead do @import 'pkg:blacklight-frontend/stylesheets/blacklight'. If this use case actually arises, we could tweak the package.json further in backwards compat ways to make this even smoother.

JS bundlers (not sass, actual npm-package-aware JS bundlers) that want to import SASS (I think this is a thing?), can do it at blacklight-frontend/stylesheets/*.

Advantages

jcoyne commented 1 month ago

@jrochkind this goes against our (Stanford's) desire to do importmap from source and not require any npm.

jrochkind commented 1 month ago

@jcoyne I do not believe it does that!

The intention here is that importmap use works exaclty the same as it did before except for changing the package name in the import -- it's still coming from the same place it was before, all that's changed is the directory name/importmap package name. npm use is not required. It's exactly the same mechanism as before, all that's changed is the name.

(The name change was required to allow npm-package-using files to use the same source without rollup, since that is the npm package name -- especially cause importmap can't use relative imports, so I needed the internal eg `import Core from 'blacklight-frontend/core' to be the same text for both -- but when you are using importmaps, this is coming from the importmap map, not from npm!).

Let me know if I've failed at this goal -- I will try to fix it if I have, I am pretty sure as long as we are willing to change the name importmap uses to blacklight-frontend it is possible to unify these with all those advantages! If I've messed up and haven't done it, I will work more to try to fix!

I am absolutely intending to support use of importmap in exactly the same way you have been. Literally all that's changed for that path is the directory name and import name. I think I have done that! Maybe you wanna try it out, or take a look at a test app generated under this branch?

jrochkind commented 1 month ago

Sorry when I said they are unified to use "same source", I mean they are both using the original source files at ./app/javascript/blacklight[-frontend]. package.json no longer has to use the rollup build, it can now use the original source files.

But import map apps are using those files through the blacklight gem and importmap-rails (in the exact same way as before this PR), and package.json apps are using those same files but packaged up in an npm package.

This does not change the manner of support for importmaps, only the directory/url/package name that gets pinned in the import map.

jrochkind commented 1 month ago

tldr another try:

For local engine resources used by importmap-rails, we change the name they are pinned as to blacklight-frontend instead of blacklight. No other changes to importmap-rails use case.

This change, to match the npm package name, means that npm-package-users can use the source files in app/javascript/blacklight instead of requiring a separate rollup output. This has a variety of advantages to npm-package-users, including: in the future eliminating the need for rollup; allowing npm-package-users to import single source files (they couldn't before); allowing npm-package-users to use local file system yarn adds, or git yarn adds (they couldn't before, because of need to run prepare script to get rollup output that is not checked into repo).

jrochkind commented 1 month ago

Thank you @jcoyne !