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.

Sign up for GitHub

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

Jump to bottom

Re: https://github.com/OpenHistoricalMap/issues/issues/929, changes t…o ohm-website to allow it to work with map-styles as an npm module. #269

Draft
wants to merge 2 commits into
base: staging
Choose a base branch
from
Draft

Re: https://github.com/OpenHistoricalMap/issues/issues/929, changes t…o ohm-website to allow it to work with map-styles as an npm module. #269

wants to merge 2 commits into from

Conversation

erictheise
Copy link
Member

This is a companion pull request to OpenHistoricalMap/map-styles#32.

@github-actions github-actions bot added the big-pr label Feb 6, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2025

1 Warning
⚠️ Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?

Generated by 🚫 Danger

@erictheise erictheise added the javascript Pull requests that update Javascript code label Feb 6, 2025
@erictheise erictheise mentioned this pull request Feb 6, 2025
Open
@danrademacher
Copy link
Member

As noted in OpenHistoricalMap/map-styles#32 (comment), we will need to ultimately host separate, fully functional style JSON files for each of the styles we host (Historical, Railway, Woodblock, Japanese Scroll) so that those styles can be easily used by other consumers, knowing they are pulling from the canonical source on openhistoricalmap.org.

danrademacher
danrademacher requested changes Feb 11, 2025
View reviewed changes
Copy link
Member

@danrademacher danrademacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this all seems fine based on reading the code. But when I try to run the code locally, I get a big ol' Rails error:

image

ohm-website-web-1  | Started GET "/" for 172.20.0.1 at 2025-02-11 01:43:39 +0000
ohm-website-web-1  | Processing by SiteController#index as HTML
ohm-website-web-1  |   Rendering layout layouts/map.html.erb
ohm-website-web-1  |   Rendering site/index.html.erb within layouts/map
ohm-website-web-1  |   Rendered site/index.html.erb within layouts/map (Duration: 5.5ms | GC: 0.0ms)
ohm-website-web-1  |   Rendered layout layouts/map.html.erb (Duration: 235.6ms | GC: 0.8ms)
ohm-website-web-1  | Completed 500 Internal Server Error in 488ms (ActiveRecord: 0.0ms (0 queries, 0 cached) | GC: 3.7ms)
ohm-website-web-1  |
ohm-website-web-1  |
ohm-website-web-1  |
ohm-website-web-1  | ActionView::Template::Error (couldn't find file 'map-styles/dist/ohm.styles' with type 'application/javascript'
ohm-website-web-1  | Checked in these paths:
ohm-website-web-1  |   /app/app/assets/config
ohm-website-web-1  |   /app/app/assets/favicons
ohm-website-web-1  |   /app/app/assets/images
ohm-website-web-1  |   /app/app/assets/javascripts
ohm-website-web-1  |   /app/app/assets/opensearch
ohm-website-web-1  |   /app/app/assets/stylesheets
ohm-website-web-1  |   /app/vendor/assets/iD
ohm-website-web-1  |   /app/vendor/assets/javascript
ohm-website-web-1  |   /app/vendor/assets/jquery
ohm-website-web-1  |   /app/vendor/assets/leaflet
ohm-website-web-1  |   /app/vendor/assets/polyfill
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-1.4.0/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-1.4.0/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/doorkeeper-5.7.1/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/doorkeeper-5.7.1/vendor/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/i18n-js-3.9.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap_form-5.4.0/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/jquery-rails-4.6.0/vendor/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/turbo-rails-2.0.11/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actiontext-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actiontext-7.2.2/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actioncable-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/activestorage-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actionview-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /app/config
ohm-website-web-1  |   /app/node_modules
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/popper_js-2.11.8/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap-5.3.3/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap-5.3.3/assets/javascripts
ohm-website-web-1  |   /app/test/javascripts
ohm-website-web-1  |   /app/test/javascripts/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-mocha-2.3.3/lib/teaspoon/mocha/assets):
ohm-website-web-1  |
ohm-website-web-1  | Causes:
ohm-website-web-1  | Sprockets::FileNotFound (couldn't find file 'map-styles/dist/ohm.styles' with type 'application/javascript'
ohm-website-web-1  | Checked in these paths:
ohm-website-web-1  |   /app/app/assets/config
ohm-website-web-1  |   /app/app/assets/favicons
ohm-website-web-1  |   /app/app/assets/images
ohm-website-web-1  |   /app/app/assets/javascripts
ohm-website-web-1  |   /app/app/assets/opensearch
ohm-website-web-1  |   /app/app/assets/stylesheets
ohm-website-web-1  |   /app/vendor/assets/iD
ohm-website-web-1  |   /app/vendor/assets/javascript
ohm-website-web-1  |   /app/vendor/assets/jquery
ohm-website-web-1  |   /app/vendor/assets/leaflet
ohm-website-web-1  |   /app/vendor/assets/polyfill
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-1.4.0/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-1.4.0/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/doorkeeper-5.7.1/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/doorkeeper-5.7.1/vendor/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/i18n-js-3.9.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap_form-5.4.0/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/jquery-rails-4.6.0/vendor/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/turbo-rails-2.0.11/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actiontext-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actiontext-7.2.2/app/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actioncable-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/activestorage-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/actionview-7.2.2/app/assets/javascripts
ohm-website-web-1  |   /app/config
ohm-website-web-1  |   /app/node_modules
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/popper_js-2.11.8/assets/javascripts
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap-5.3.3/assets/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/bootstrap-5.3.3/assets/javascripts
ohm-website-web-1  |   /app/test/javascripts
ohm-website-web-1  |   /app/test/javascripts/stylesheets
ohm-website-web-1  |   /var/lib/gems/3.1.0/gems/teaspoon-mocha-2.3.3/lib/teaspoon/mocha/assets)
ohm-website-web-1  |     1: <% content_for :head do %>
ohm-website-web-1  |     2:   <%= javascript_include_tag "index" %>
ohm-website-web-1  |     3: <% end %>
ohm-website-web-1  |     4:
ohm-website-web-1  |     5: <% content_for(:body_class) { "map-layout" } %>
ohm-website-web-1  |
ohm-website-web-1  | app/views/layouts/map.html.erb:2
ohm-website-web-1  | app/views/layouts/map.html.erb:1
ohm-website-web-1  | config/initializers/compressed_requests.rb:27:in `call'
ohm-website-web-1  | config/initializers/cors.rb:9:in `call'

this is specific to pages with the map on it. About, Copyright, other mapless pages work fine. Homepage, /history, /export all fail with the same error

@erictheise
Copy link
Member Author

Did you do a yarn install or rebuild the website container?

@danrademacher
Copy link
Member

Did you do a yarn install or rebuild the website container?

I just did my usual practice of switching to the branch/PR I want to test and running docker compose up --build which typically "just works" (though it can take a while).

To confirm, I checked out staging, and confirmed that the same pages that fail on this branch work as expected on staging

Is there some additional step required to run yarn install in a separate CLI for the web container or something?

@erictheise
Copy link
Member Author

Do you have an @openhistoricalmap/map-styles subdirectory under node_modules? If not, yarn install should pick it up. Or you could try running docker compose up --build --no-cache and see if that gets it. I don't run --build that often but when I do I find that often it doesn't pick up fresh changes due to caching.

If the module's been installed then it's a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danrademacher danrademacher danrademacher requested changes

@tsinn tsinn Awaiting requested review from tsinn

@1ec5 1ec5 Awaiting requested review from 1ec5

@vknoppkewetzel vknoppkewetzel Awaiting requested review from vknoppkewetzel

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
big-pr javascript Pull requests that update Javascript code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erictheise @danrademacher