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

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

Merged
merged 1 commit into from Jan 22, 2024

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Jan 18, 2024

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.

…rted from blacklight-frontend npm package

eg local app might

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

or

    import Blacklight from 'blacklight-frontend/app/javascript/blacklight/core'
    import Modal from 'blacklight-frontend/app/javascript/blacklight/modal';
@jrochkind jrochkind force-pushed the blacklight_frontend_individual_sources branch from 60c6267 to d8c0ec4 Compare January 18, 2024 20:55
@jcoyne
Copy link
Member

jcoyne commented Jan 19, 2024

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

@jrochkind
Copy link
Member Author

jrochkind commented Jan 22, 2024

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

jrochkind commented Jan 22, 2024

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

jcoyne commented Jan 22, 2024

Thanks for checking that.

@jrochkind jrochkind merged commit 2052685 into main Jan 22, 2024
13 checks passed
@jrochkind jrochkind deleted the blacklight_frontend_individual_sources branch January 22, 2024 17:22
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.

Should it be possible to selectively import blacklight-frontend modules in Blacklight 8, without importmaps?
2 participants