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

Revert "all imports should be relative, so individual files can still… #3164

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

corylown
Copy link
Contributor

@corylown corylown commented Jun 3, 2024

… be imported from blacklight-frontend npm package"

This reverts commit d8c0ec4.

This change broke the javascript imports in all our Blacklight apps.
Screenshot 2024-06-03 at 3 21 02 PM

… be imported from blacklight-frontend npm package"

This reverts commit d8c0ec4.
@jrochkind
Copy link
Member

Thanks for pointing out the revert PR @corylown !

Can you show me what your BL imports looked like, and tell me if you were using importmaps-rails or other?

The problem for me is that this breaks my imports, makes it impossible(?) to import what I need with a conventional JS pipeline like esbuild, rollup, or vite. Or at least in the way I was trying to. I don't totally understand what is going on.

Is it possible we could change this in upcoming BL9 in a way that might require adjusting imports (it is a major release after all), but would work for both of us?

I am willing to work on it -- or come up with other workarounds that aren't this revert but solve my problem in other ways, I have some ideas -- but I don't know what it is that broke your app since it passed CI and I can't reproduce, so I don't have a way to tell me if my fixes are good for you/others and won't be reverted!

Would you be willing to talk to to me about it?

Other people that may be interested in using BL8 and BL9 ESM with a conventional npm-package-and-esm-based JS pipeline like vite or esbuild may include @tpendragon and @bess

@seanaery
Copy link
Contributor

Just a quick note that the same thing that Stanford experienced happened to us at Duke -- the relative ./ imports broke all the JS in our BL8/arclight app, which uses Rails 7 & Blacklight 8 default importmap-rails for JS assets, e.g., see application.js.

@corylown
Copy link
Contributor Author

@jrochkind we're using importmap-rails and propshaft. What I'm seeing in the browser console when the import paths are relative is that the browser is trying to request the file without the digest, for instance /assets/blacklight/modal.js instead of /assets/blacklight/modal-badfca3c.js. There may be a way to work around this by pinning correctly in importmap.rb, but I haven't sorted that out yet.

@jrochkind
Copy link
Member

jrochkind commented Oct 10, 2024

OK thanks, that info was helpful @corylown !

I guess that was with some version of BL 8 from before this Revert was merged, ok.

When trying to work on this further, I'll make sure to test with propshaft/importmap-rails and see if I can reproduce, or if it seems to work. If I have a branch off main that seems to work well with propshaft/importmap-rails when I generate a new app the same way the CI does with those things -- I may reach out and ask if anyone else can test to see if there's something different in your app that breaks it (and if so, if there's a way to fix it!)

I guess if you are using propshaft, does that mean you have a package.json and are using esbuild for your bootstrap stuff?

@seanaery -- thanks! Do you also use propshaft like @corylown , or are you using sprockets still?

@corylown
Copy link
Contributor Author

For bootstrap we have a package.json for bundling the css. The bootstrap javascript is pinned to the npm package and is delivered with importmaps.

Happy to test approaches to this as I have time. Feel free to reach out.

@seanaery
Copy link
Contributor

@jrochkind We're still using Sprockets with Importmap; haven't used Propshaft just yet. So we don't currently have a package.json or need to use yarn for our BL8 app.

@jrochkind
Copy link
Member

Thanks y'all this is all helpful

@jrochkind
Copy link
Member

Yep, turns out the way I was trying to do things was incompatible with importmap/importmap-rails -- you can't really use relative imports with importmap-rails.

To make matters more confusing, it only broke things in production for importmap-rails users, and not development/test -- is why the tests didn't catch it. :(

The point of this was trying to get the source files to work both for node/npm apps, and importmap apps.

I found another approach to this, which will really work for both, and just requires importmap-using apps to switch the name/path BL JS is pinned under in BL9, a backwards incompat that just requires a simple substitution in your local app JS files. Merged at #3371

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.

5 participants