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

Support Sprockets 4 #1452

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Support Sprockets 4 #1452

merged 1 commit into from
Oct 28, 2019

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Oct 25, 2019

Objective

This PR should fix the CI build, which is currently failing for all appraisals with the following error:

Sprockets::Railtie::ManifestNeededError:
  Expected to find a manifest file in `app/assets/config/manifest.js`
  But did not, please create this file and use it to link any assets that need
  to be rendered by your app:

  Example:
    //= link_tree ../images
    //= link_directory ../javascripts .js
    //= link_directory ../stylesheets .css

Details

This is due to Sprockets 4, which requires this manifest.js file. Sprockets 4 was published on 8 October 2019, the day after the last successful build.

From what I can tell, the only fix is having the client app listing Administrate's asset files in its manifest.js file. Therefore I'm creating one for the example app (used in the specs) and documenting this extra manual work in the readme.

For some detail on this manifest file, see https://www.schneems.com/2017/11/22/self-hosted-config-introducing-the-sprockets-manifestjs/

But it passes on my machine!

If you run the specs locally with bundle exec rake, they'll probably pass. This is because the bundled Gemfile.lock has Sprockets pinned to version 3.7.2 currently. However if you run the appraisal specs with bundle exec appraisal rake, these may fail because the appraisal lockfiles are not under version control, and therefore will be regenerated on first run.

Note that a newly created Rails app won't have this issue at the time being. This is because Rails apps are still being generated to use the sass gem (despite its being end-of-life'd) and this depends on sprockets < 4. Using sassc in your app may land you Sprockets 4 though (you'd still have to explicitly bundle update to get it).

Future options

I don't see a way to have Administrate provide an engine-level manifest.js instead of having the client application manually add it. I might be missing something though.

Another option would be having the Administrate generator add the required lines to the manifest files. I think we'd still have to document it in the readme, as that's bound to not be enough for many.

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Cannot find module '@thoughtbot/eslint-config'
Cannot find module '@thoughtbot/eslint-config'
Referenced from: .eslintrc
Error: Cannot find module '@thoughtbot/eslint-config'
    at ModuleResolver.resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:479:28)
    at load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:551:26)
    at configExtends.reduceRight (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:425:36)
    at Array.reduceRight ()
    at applyExtends (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:403:26)
    at loadFromDisk (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:523:22)
    at Object.load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:559:20)
    at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:227:44)
    at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:179:43)

@pablobm
Copy link
Collaborator Author

pablobm commented Oct 25, 2019

Argh, the build fails because of the security advisory on loofah. Both #1449 and this need to go through together for the build to pass.

@pablobm pablobm mentioned this pull request Oct 25, 2019
This fixes CI, which is currently failing for all appraisals with the
following error:

```
Sprockets::Railtie::ManifestNeededError:
  Expected to find a manifest file in `app/assets/config/manifest.js`
  But did not, please create this file and use it to link any assets that need
  to be rendered by your app:

  Example:
    //= link_tree ../images
    //= link_directory ../javascripts .js
    //= link_directory ../stylesheets .css
```

This is due to Sprockets 4, which requires this `manifest.js` file. Sprockets
4 was published on 8 October 2019, the day after the last successful build.

From what I can tell, the only fix is having the client app listing
Administrate's asset files in its `manifest.js` file. Therefore I'm creating
one for the example app (used in the specs) and documenting this extra manual
work in the README.

For some detail on this manifest file, see
https://www.schneems.com/2017/11/22/self-hosted-config-introducing-the-sprockets-manifestjs/

If you run the specs locally with `bundle exec rake`, they'll probably pass.
This is because the bundled `Gemfile.lock` has Sprockets pinned to version
3.7.2 currently. However if you run the appraisal specs with
`bundle exec appraisal rake`, these may fail because the appraisal lockfiles
are not under version control, and therefore will be regenerated on first run.

Note that a newly created Rails app won't have this issue at the time being.
This is because Rails apps are still being generated to use the `sass` gem
(despite its being end-of-life'd) and this depends on `sprockets < 4`. Using
`sassc` in your app may land you Sprockets 4 though (you'd still have to
explicitly `bundle update` to get it).

I don't see a way to have Administrate provide an engine-level `manifest.js`
instead of having the client application manually add it. I might be missing
something though.

Another option would be having the Administrate generator add the required
lines to the manifest files. I think we'd still have to document it in the
README, as that's bound to not be enough for many.
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Cannot find module '@thoughtbot/eslint-config'
Cannot find module '@thoughtbot/eslint-config'
Referenced from: .eslintrc
Error: Cannot find module '@thoughtbot/eslint-config'
    at ModuleResolver.resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:479:28)
    at load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:551:26)
    at configExtends.reduceRight (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:425:36)
    at Array.reduceRight ()
    at applyExtends (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:403:26)
    at loadFromDisk (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:523:22)
    at Object.load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:559:20)
    at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:227:44)
    at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:179:43)

@nickcharlton nickcharlton merged commit df5a1f1 into thoughtbot:master Oct 28, 2019
@pablobm pablobm deleted the sprockets-4 branch June 24, 2021 10:39
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