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

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

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Oct 17, 2024

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

  • Both npm-package-using and importmap-using apps will be using the same common source at app/javascript/blacklight-frontend, simpler, more consistent.
  • Rollup build step is now only necessary for sprockets, when we stop supporting sprockets we could get rid of rollup build step entirely.
  • application.js can now be identical between importmap case and npm-package-using case. Which could make our generators more maintainable and simpler. Also makes it easier for apps to switch back and forth between importmap and npm-package-use without having to change their .js source files at all. Probably makes it less confusing for developers, making it clear what the importmap vs npm-package choices actually means.
  • npm-package-using apps can now import specific JS files individually, like core.js, bookmark_toggle.js, etc. They could not do this before!
  • npm-package-using apps can now point directly at git branches in their package.json (ie yarn add blacklight-frontend@git:https://github.com/projectblacklight/blacklight#branch_name -- this could not work before this PR, becuase yarn doesn't run prepare on git checkout, so the rollup-created files that were the only ones that worked were not available! This made CI or using pre-release npm package infeasible.

@jrochkind jrochkind force-pushed the experiment_js_unity branch from e4340d1 to f1f881e Compare October 18, 2024 14:04
@jrochkind jrochkind changed the title WIP experiment please ignore unify importmap and npm use on 'blacklight-frontend', modernize package.json with exports Oct 18, 2024
@jcoyne
Copy link
Member

jcoyne commented Oct 18, 2024

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

@jrochkind
Copy link
Member Author

jrochkind commented Oct 18, 2024

@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
Copy link
Member Author

jrochkind commented Oct 18, 2024

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
Copy link
Member Author

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 jrochkind marked this pull request as ready for review October 29, 2024 19:04
@jrochkind
Copy link
Member Author

Thank you @jcoyne !

@jrochkind jrochkind merged commit 7b0797d into main Oct 29, 2024
13 checks passed
@jrochkind jrochkind deleted the experiment_js_unity branch October 29, 2024 19:41
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.

2 participants