Updated package.json and added font-awesome-rails gem #658

waridrox · 2021-06-20T17:04:11Z

Part of #645
Ok so the package.json file had spectral-workbench as the project name which conflicts with the spectral-workbench.js library currently hosted on npm. As of now, it is named as spectralworkbench which could be renamed to something meaningful later 😅.

Plus there's an inclusion of font-awesome-rails gem that provides access to the font-awesome lib. The goal is to rely as little as possible on CDN services...

The Gemfile.lock works fine locally but need to see if it works on the live version
CC: @jywarren @pydevsg
Thanks!

gitpod-io · 2021-06-20T17:04:14Z

waridrox · 2021-06-25T16:08:51Z

Hi @jywarren @pydevsg @RuthNjeri. Pls could you review this pull request. Thanks.

This PR aims to create a package.json file and incorporate dependency files and fonts into assets folder that were available only in bower like bootstrap-css.

This shouldn't be merged straightaway though!!!
That's because some of the files like configuration depend on #660 too

The dependencies are distributed on different files. Hence all the changes should be done like #661 to retain the functionality with the new change.

RuthNjeri · 2021-06-27T12:12:55Z

Hi @waridrox 👋🏾

My first concern is all the tests in this PR failing, it seems to relate to the updates. I see a package.json file, should there be a lock file? Similar to the Rails gemfile?

Also, maybe to speed off the merging of one of the issues being solved here, one idea is to split the work focusing on the font awesome and the other handling the package.json file it would also help narrowing down on what is causing the tests to fail.

waridrox · 2021-06-27T14:34:37Z

Hi @RuthNjeri 🙌, thanks for pointing out the missing package-lock.json file, but there are different things that are related to each other and if done in a single PR could make it messy.

For instance, the current PR adds the bower dependencies which are not available on npm packages and the primary font into assets folder.
But the path for these assets to get utilised is not changed since that would require changing of paths into different files as well. That's because the paths to the files that are being utilised assume that the bower dependencies are already installed.
Though I've updated the PR to account for the changes which were done in a separate PR, the tests are still failing. Maybe the tests were written taking into account the old bower dependencies 🤔

The app works fine on gitpod when the following commands are executed in order -

bundle update --latest
bundle install
npm install
rake db:create
rake db:migrate
rails s

pydevsg · 2021-06-27T19:39:46Z

Hi, @waridrox .
Thanks @RuthNjeri for helping out initially :)
Great work on configuring the updated package.json file .
So, far looks good to me 🎉
The tests are failing as the default tests that are written before needs to be changed as well.
Rest looks good to me.

RuthNjeri · 2021-06-29T13:04:53Z

Thanks @waridrox and @pydevsg, glad to hear that everything is working as expected. Just to make sure I have understood you correctly, the bower dependencies not being available in npm, might that be affecting the Github actions set up? Could it be trying to download it?

When you mention that you haven't updated the paths to access the Bower dependency, that could be another factor for the failing setup..

The setup needs to pass first in order for the tests to run

From what you are saying, @waridrox, it seems the setup won't pass until files have the updated path to bower and also, it should be available in npm?

waridrox · 2021-06-29T13:18:39Z

Sorry for the confusion @RuthNjeri 😅, in an attempt to move away from bower dependencies, I have extracted the dependable files (available/deprecated as bower dependencies) and put them in the assets folder, thereby changing the path in the files that require those dependencies.

Could it be trying to download it?

Sure then I'll look into the tests and make tweaks accordingly. Thanks :)

jywarren · 2021-06-29T14:26:28Z

Just chiming in here -- great start!

font-awesome-rails

I see we're doing the same here as in plots2, which looks good:

https://github.com/publiclab/plots2/blob/4756f69a856e0328a2ce60a60778bced74d9d370/Gemfile#L73

jywarren

Hi @waridrox -- left a lot of comments but I think the overall is that we could simplify this considerably and narrow the possible errors by sticking with the existing versions of all the package.json modules, and where they don't exist or need to be replaced, looking to the plots2 repository for the version we used there. However, swapping in other versions is likely to work mainly for CSS or image/font assets, and not for JS, which would be likely to break.

Can we try a narrower set of changes and see how it does, for example sourcing the NPM module for Junction font, and reverting most or all of the package.json version changes?

jywarren · 2021-06-29T14:29:06Z

Gemfile.lock

-      activerecord (= 5.2.4.3)
-      activestorage (= 5.2.4.3)
-      activesupport (= 5.2.4.3)
+    rails (5.2.4.6)


oh, these are some pretty big changes here unrelated to the new gem. But let's see, maybe that wasn't an issue. In general, though, perhaps we should do what we can to limit the upgrades to just those directly related to the issue at hand... makes it easier to debug and review as well!

jywarren · 2021-06-29T14:30:14Z

app/assets/fonts/webfonts/junction-light.svg

@@ -0,0 +1,553 @@
+<?xml version="1.0" standalone="no"?>


OK, so all the Junction fonts are available on NPM here: https://github.com/publiclab/plots2/blob/main/package.json#L45

Shall we switch to using that?

jywarren · 2021-06-29T14:30:46Z

app/assets/javascripts/application.js

-//= require bootstrap-css/js/bootstrap.min.js
-//= require getusermedia.js/dist/getUserMedia.min.js
+//= require bootstrap.min.js
+//= require getusermedia-js/dist/getUserMedia.min.js


Great, yes, this looks like typical changes required when moving from Bower to NPM. Great!!

jywarren · 2021-06-29T14:31:42Z

app/assets/javascripts/bootstrap.min.js

@@ -0,0 +1,6 @@
+/*!


Here, I don't think actually importing the whole library is ideal. In general we want to avoid that at all costs, in fact!

However, looking at plots2 we have a good alternative NPM module here: https://github.com/publiclab/plots2/blob/4756f69a856e0328a2ce60a60778bced74d9d370/package.json#L26

jywarren · 2021-06-29T14:32:34Z

app/views/layouts/application.html.erb

@@ -19,7 +19,6 @@
    <link rel="shortcut icon" href="//spectralworkbench.org/images/spectral-workbench-256.png" />

    <%= stylesheet_link_tag    "application", :media => "all" %>
-    <link rel="stylesheet" href="/lib/fontawesome/css/font-awesome.min.css"/>


Is this because we're going to be compiling them into the main stylesheet, or making it appear in an asset path that's searched, so it's auto-included from line 21?

Yes, the font-awesome gem enables the use of the lib since that is already included in the application.css file.

jywarren · 2021-06-29T14:33:21Z

config/application.rb

@@ -54,6 +54,8 @@ class Application < Rails::Application

    # Enable the asset pipeline
    config.assets.enabled = true
+    config.assets.paths << Rails.root.join("app", "assets", "fonts")
+    config.assets.paths << Rails.root.join("node_modules")
    config.assets.paths << Rails.root.join('public/lib')


So yes, here is my preference, especially for fontawesome and things that have fonts, stylesheets, or image assets, which we may need to be able to access via a public web route. But cool to see the above options added too. Thanks!

Sure, now there's no need of this, assets can be accessed at public/lib path with yarn

jywarren · 2021-06-29T14:33:38Z

config/initializers/assets.rb

@@ -10,6 +10,8 @@
  # Rails.application.config.assets.paths << Emoji.images_path
  # Version of your assets, change this if you want to expire all your assets
  config.assets.paths << Rails.root.join('public/lib')
+  config.assets.paths << Rails.root.join("node_modules")
+  config.assets.paths << Rails.root.join("app", "assets", "fonts")


Is it required to do it twice?

jywarren · 2021-06-29T14:35:35Z

package.json

  "dependencies": {
-    "ace-builds": "jywarren/ace-builds#v1.2.4",


Watch out - here, this looks like I customized it before including it. We should stick with the version I had pegged to, just in case. For the remainder, I worry about the versions you've chosen... some are major version number upgrades (see https://docs.npmjs.com/about-semantic-versioning/) and are likely to break the app. Was there a reason to attempt upgrades of all these modules at once? It could be much easier to go one by one and check each time.

So wherever we can't find a published NPM module, the syntax is that it will search as follows:

jywarren/ace-builds#v1.2.4 means:

https://github.com/jywarren/ace-builds with branch or commit v1.2.4

So that would be:

https://github.com/jywarren/ace-builds/tree/v1.2.4

We can point at non-NPM-published code in this way, as long as we specify a commit or a published release in this syntax. Just in case you run into issues, but I think all of these were already pegged to repositories rather than NPM modules, already, when we originally migrated from Bower!

jywarren · 2021-06-29T14:36:20Z

package.json

+    "bootstrap": "^3.4.1",
+    "d3": "^3.5.17",
+    "flot": "^0.8.0-alpha",
+    "getusermedia-js": "^1.0.0",


Be careful that this package could be entirely different from the one specified before - by a different author, etc. Maybe best stick with the existing one for now?

jywarren · 2021-06-29T14:53:06Z

In fact, i see that the error is still error Lockfile does not contain pattern: "ace-builds@^1.4.12" -- what I think happened is that you a) left out the lockfile, b) changed the package.json dependency entries and version numbers, c) generated a new package-lock.json file, d) added the lockfile

So, if you revert the package.json file, then generate a new lockfile with npm install and check that in, you should get past this error.

THAT SAID, we are using Yarn in this repo. So the lockfile really ought to be yarn.lock, right?

Did it really error asking for package-lock.json the first time? I believe Yarn wants just a yarn.lock file (https://snyk.io/blog/what-is-package-lock-json/, https://classic.yarnpkg.com/blog/2018/06/04/yarn-import-package-lock/) so we may be confusing it by adding both?

We highly recommend you to delete the package-lock.json file if you decide to use yarn in order to avoid future confusion and possible consistency issues. (from above yarnpkg.com article)

What was the exact error when it asked for package-lock.json, when there was only a yarn.lock available?

Yes - it was asking why packages weren't showing in the lockfile, but not package-lock.json - presumably it meant yarn.lock:

error Lockfile does not contain pattern: "ace-builds@^1.4.12"

And it was happening during this yarn check step that precedes yarn install:

spectral-workbench/.github/actions/setup-test-environment/action.yml

Line 18 in fbae2af

run: yarn check || yarn install --frozen-lockfile;

My guess is that you had modified the package.json file already at that point, and it couldn't find the corresponding update to the yarn.lock file. But it's for a package which as I noted above, I don't think we need to change!

Hope this helps. Maybe we just need a much narrower set of changes to start this PR?

jywarren · 2021-06-29T18:41:52Z

OK, so let's try splitting this into separate PRs. One for changing the name in package.json to spectral-workbench-rails, one for adding the font-awesome-rails gem (which we could maybe skip, if it's working fine purely through package.json?) and one for each other issue we're hoping to solve - for example, the Junction font, if that's broken. But let's each time cite the error we're trying to address with the change, so we remain focused on just that one issue in the PR. How does that sound?

This is exciting, great start, @waridrox!!!

waridrox · 2021-07-02T12:37:13Z

Done in #671

jywarren · 2021-07-06T16:50:23Z

Very nicely done. thank yoU!

Updated package.json and added font-awesome-rails gem

5e8aec2

waridrox force-pushed the dependencies branch from 09fd6e5 to 5e8aec2 Compare June 25, 2021 11:30

waridrox added 2 commits June 25, 2021 17:26

Handled pages utilising assets

9840133

Addding bower deps into assets

7337eb0

waridrox mentioned this pull request Jun 25, 2021

[Planning] - spectralworkbench.js / rails integration #645

Closed

11 tasks

waridrox requested review from jywarren, pydevsg and RuthNjeri June 25, 2021 16:36

configuring path for assets and node_modules

8ac8aa9

waridrox added 2 commits June 27, 2021 18:52

Added package-lock.json and static background img for /v2

3aec32b

Changing paths for new dependencies

fe20d52

jywarren requested changes Jun 29, 2021

View reviewed changes

jywarren mentioned this pull request Jun 29, 2021

Upgrade to SWB.js v0.2.0 #668

Merged

checking for yarn.lock

c50e9f1

waridrox closed this Jun 30, 2021

waridrox reopened this Jun 30, 2021

waridrox closed this Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated package.json and added font-awesome-rails gem #658

Updated package.json and added font-awesome-rails gem #658

waridrox commented Jun 20, 2021

gitpod-io bot commented Jun 20, 2021

waridrox commented Jun 25, 2021 •

edited

Loading

RuthNjeri commented Jun 27, 2021

waridrox commented Jun 27, 2021 •

edited

Loading

pydevsg commented Jun 27, 2021

RuthNjeri commented Jun 29, 2021

waridrox commented Jun 29, 2021

jywarren commented Jun 29, 2021

jywarren left a comment

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

waridrox Jul 1, 2021

jywarren Jun 29, 2021

waridrox Jul 1, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren Jun 29, 2021

jywarren commented Jun 29, 2021

jywarren commented Jun 29, 2021

waridrox commented Jul 2, 2021

jywarren commented Jul 6, 2021

		"dependencies": {
		"ace-builds": "jywarren/ace-builds#v1.2.4",

Updated package.json and added font-awesome-rails gem #658

Updated package.json and added font-awesome-rails gem #658

Conversation

waridrox commented Jun 20, 2021

gitpod-io bot commented Jun 20, 2021

waridrox commented Jun 25, 2021 • edited Loading

RuthNjeri commented Jun 27, 2021

waridrox commented Jun 27, 2021 • edited Loading

pydevsg commented Jun 27, 2021

RuthNjeri commented Jun 29, 2021

waridrox commented Jun 29, 2021

jywarren commented Jun 29, 2021

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jun 29, 2021

jywarren commented Jun 29, 2021

waridrox commented Jul 2, 2021

jywarren commented Jul 6, 2021

waridrox commented Jun 25, 2021 •

edited

Loading

waridrox commented Jun 27, 2021 •

edited

Loading