Skip to content

Remove webpack#866

Closed
faustinoaq wants to merge 28 commits into
masterfrom
fa/remove-webpack
Closed

Remove webpack#866
faustinoaq wants to merge 28 commits into
masterfrom
fa/remove-webpack

Conversation

@faustinoaq
Copy link
Copy Markdown
Contributor

@faustinoaq faustinoaq commented Jun 16, 2018

DO NOT MERGE YET! ⚠️

This highly depends on #865 and #864

Also please be aware of all the breaking changes

No NPM means, we need to use vanilla JS and plain CSS.

Maybe we can propose a --npm flag, WDYT?

Description of the Change

⚠️ This PR is optional, because to remove webpack we require to use plain JS and CSS without npm and sass dependencies.

This PR depends on #865

Alternate Designs

Keep configurable/removable webpack/sass dependency implemented on #865

Benefits

  • Removes amber.min.js dependency, see: Update assets #825
  • No more NPM vulnerabilities by default, see Github flagging hapijs for vulnerability #834
  • Use plain JS and CSS without dependencies
  • No opinionated frontend, so, front-end devs can use whatever tools they want (react, yarn, rollup, parcel, etc) without default bundler config.
  • No require NPM in amber generated projects, so we'd remove npm from amber Dockerfile:

amber/Dockerfile

Lines 7 to 9 in ddcb6f2

# Install Node
RUN curl -sL https://deb.nodesource.com/setup_6.x | bash -
RUN apt-get install -y nodejs

Possible Drawbacks

  • The plain JS is using ES5 to be compatible with most browsers
  • Sass file has been replaced by plain CSS (so, no color variables here)
  • To copy assets into public/ folder, amber watch is using auxiliar tasks with cp and cat commands

@faustinoaq faustinoaq requested a review from fridgerator June 16, 2018 20:47
Copy link
Copy Markdown
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

Do we want to remove webpack or do we want to remove the dependency? I actually like having webpack included out of the box, but I want the ability to remove/replace it if so desired. wdyt?

},
resolve: {
alias: {
amber: path.resolve(__dirname, '../../lib/amber/assets/js/amber.js')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will need to figure out how to include this for anyone using web sockets.

database: <%= @database %>
language: <%= @language %>
model: <%= @model %>
watch:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

awesome! 💯

context.response.print(error.internal_server_error)
end
end
class <%= class_name %> < Amber::Pipe::Error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't seem anything to do with removing web pack.

Copy link
Copy Markdown
Contributor

@eliasjpr eliasjpr left a comment

Choose a reason for hiding this comment

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

I am confused about this PR in the sense that is Removing something but it adds more 1700+ lines 😕 Other than that Webpack can be 💣

@faustinoaq
Copy link
Copy Markdown
Contributor Author

@eliasjpr This PR is not ready yet, (I just update the PR description) This highly depends on #865 and #864

@robacarp robacarp closed this Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants