Skip to content

Make webpacker build available in production#7915

Merged
mrcasals merged 6 commits intodevelopfrom
chore/add-webpacker-rake-task
Apr 29, 2021
Merged

Make webpacker build available in production#7915
mrcasals merged 6 commits intodevelopfrom
chore/add-webpacker-rake-task

Conversation

@beagleknight
Copy link
Copy Markdown
Contributor

@beagleknight beagleknight commented Apr 29, 2021 •
edited
Loading

🎩 What? Why?

To make the webpacker build available in production environments we should add a rake task that compiles the assets when we run bundle exec rake assets:precompile.

In this PR I also included some changes related to the deployment test I did:

  • I replaced yarn with npm again 😅 (see Update js dependencies using npm #1628). I was having problems in my Heroku environment and I decided to go back to npm. This is something that we can discuss with @decidim/mantainers, but I would like to give my opinion: even if yarn is the package manager that come with Rails by default I strongly advise to use the official one: npm.
  • The package-lock.json (or yarn.lock) should be included in the gem.
  • Some assets were removed accidentally in Migrate to Webpacker #7464 . Even if they will be restored in Migrate to Webpacker (CSS and images) #7733 it is better to have them available at the moment in the old asset pipeline.
  • I locked the webpacker version to "6.0.0.beta.7" and there are two good reasons for this: we should always use released versions and for some reason if you try to use "~> 6.0.0.beta.7" it always install a pre.2 version. It could be related to some strange order in https://rubygems.org/gems/webpacker
  • I moved all JS dependencies to the dependencies block. The reason for that is when you are in a production environment usually the NODE_ENV variable is set to production. That means that if you run npm install you don't install the devDependencies making the build impossible. Since it is not a Node.js application it doesn't make any sense having dependencies and devDependencies. A possible workaround is specifying the NODE_ENV to development when building the assets, but I don't feel ok about that.

And that's it! @decidim/mantainers feel free to discuss anything related to this 😄 . After merging this PR we will be able to create review apps again and I strongly advise to have it working as soon as possible.

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@beagleknight beagleknight mentioned this pull request Apr 29, 2021
Closed
qlty-cloud-legacy[bot]
qlty-cloud-legacy bot reviewed Apr 29, 2021
View reviewed changes
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 30302 lines exceeds the maximum allowed for the inline comments feature.

@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Apr 29, 2021

👍🏾 to merge this before #7733 and I'll adapt that PR, now I'm in the middle of what we discussed in the comments and I'm a bit stuck.

BTW I've no issues in changing to npm, but there are some parts of Rails that won't be as smooth (i.e the script bin/update relies on yarn, although it can be updated)

qlty-cloud-legacy[bot]
qlty-cloud-legacy bot reviewed Apr 29, 2021
View reviewed changes
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 30303 lines exceeds the maximum allowed for the inline comments feature.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 29, 2021

@beagleknight thanks for your work! We already discussed this offline, I think the current approach is fine. Apparently, GitHub Actions are all updated, right?

Thanks @ferblape for reviewing this, too!

@mrcasals mrcasals merged commit 53802b5 into develop Apr 29, 2021
@mrcasals mrcasals deleted the chore/add-webpacker-rake-task branch April 29, 2021 12:33
@mrcasals mrcasals mentioned this pull request Apr 29, 2021
Merged
@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Apr 30, 2021

@beagleknight @mrcasals what version of npm are you using? Mine is 7.6.2 which updates the lockVersion of package-lock.json. What version are you using? Or should be upgrade decidim to use the latest?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 30, 2021

I'm using 6.14.11 @ferblape but we should update if there's a newer version. Could you take care of this? 🙏

@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Apr 30, 2021 via email

@beagleknight
Copy link
Copy Markdown
Contributor Author

beagleknight commented Apr 30, 2021

@beagleknight @mrcasals what version of npm are you using? Mine is 7.6.2 which updates the lockVersion of package-lock.json. What version are you using? Or should be upgrade decidim to use the latest?

I was using Node 14.x and npm 6.x. I updated our staging application with those https://github.com/codegram/decidim-staging/blob/main/package.json#L21. +1 to upgrade to Node 15.x and npm 7.x (it is much faster 😄 ).

@ferblape Can you update the engines section in the package.json as well? Thanks!

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 30, 2021

Can I do it in my branch or in develop?

@ferblape as you prefer, whatever works better for you!

@ferblape
Copy link
Copy Markdown
Contributor

ferblape commented Apr 30, 2021

Ok thanks, I'll do it in my branch because I have to solve a few conflicts there.

  • node: update version to 15.14.0
  • npm: update version to 7.7.x
  • and update the documentation I created to remove the references to yarn

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 30, 2021

Awesome, thank you @ferblape! ❤️

entantoencuanto added a commit that referenced this pull request Apr 30, 2021
@entantoencuanto
Merge branch 'develop' into active_storage_migration
c9d64f5 
* develop:
  Remove creation date from meeting card (#7922)
  Use NPM instead of yarn on CI (#7919)
  Validate nickname using correct regexp (#7900)
  Make webpacker build available in production (#7915)
  New Crowdin updates (#7911)
  Open attachments in new tab (#7912)
  Fix JS errors in the admin panel (#7903)
  Fix editor: remove br tags from inside a tags (#7901)
  Authorizable comment action for proposals (#6916)
  NoMethodError raised when voting comments from threads (#7880)
  Fix not signed in needs permission redirect for internal links (#7890)
  Fix meeting registrations questionnaire free text choice answers export (#7892)
  Store election verifiable results data in election (#7882)
  New Crowdin updates (#7884)
@andreslucena andreslucena mentioned this pull request Oct 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@qlty-cloud-legacy qlty-cloud-legacy[bot] qlty-cloud-legacy[bot] left review comments

@mrcasals mrcasals mrcasals approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

target: developer-experience

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@beagleknight @ferblape @mrcasals