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

Node module #706

Merged
merged 22 commits into from Aug 30, 2017
Merged

Node module #706

merged 22 commits into from Aug 30, 2017

Conversation

@javan
Copy link
Member

javan commented Aug 28, 2017

Moves the bulk of our webpack config into a node module instead of copying it all to apps on webpacker:install. There are only four files copied to config/webpack/ now:

1 config/webpack/environment.js for setting up base configuration:

const { environment } = require('webpacker')
module.exports = environment

2-4 config/webpack/{development,production,test}.js, which build off environment.js and are identical to start:

const environment = require('./environment')
module.exports = environment.toWebpackConfig()

TODO

Before merging

  • Relax dependency versions?
  • Update docs

After merging

  • Publish to npm
    • $ npm version major
    • $ npm publish --access public

Fixes #649
Closes #264

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 28, 2017

@javan Just briefly looked at your changes and it's looking really good 👍 💯

Happy to help with docs if you like or perhaps we can do it in separate PR i.e. move all README contents into docs/ folder or something. I can do that in parallel.

For node_modules, seems like someone already got https://www.npmjs.com/package/webpacker registered 😞

@javan
Copy link
Member Author

javan commented Aug 29, 2017

@gauravtiwari, I added new documentation and touched up some of the existing bits in 34142b1. Let me know if I missed anything critical.

javan added 4 commits Aug 29, 2017
@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 29, 2017

@javan Sure thing, reviewing now. The API looks really nice though 💯

We haven't published the package to npm yet right?

@javan
Copy link
Member Author

javan commented Aug 29, 2017

Right. Not published yet. Will eventually be published as a scoped package: @rails/webpacker.

Side note: We should gut the README as soon as this is merged.

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 29, 2017

@javan Cool 👍

Yepp, will do. Thinking we create a docs/ folder and create .md for each topic. What do you think?

@javan
Copy link
Member Author

javan commented Aug 29, 2017

We can discuss more in #645, but basically I think we should remove everything that's webpack-not-webpacker related.

@javan
Copy link
Member Author

javan commented Aug 29, 2017

@gauravtiwari, can you please try this out in a real app?

You can do:

# Gemfile
gem 'webpacker', github: 'rails/webpacker', branch: 'node-module'
// package.json
"dependencies": {
  "webpacker": "rails/webpacker#node-module"
}

and then rails webpacker:install

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 29, 2017

@javan Yepp doing that now.

BTW, is code like this intentional i.e. if block and rest of the code, no space?

 const { extensions } = config
  if (!extensions.length) {
    throw new Error('You must configure at least one extension to compile in webpacker.yml')
  }
  return extensions.length === 1 ? `**/${extensions[0]}` : `**/*{${extensions.join(',')}}`

I corrected a few and if you don't mind I can push :)

@javan
Copy link
Member Author

javan commented Aug 29, 2017

Hmm, not seeing that locally or on github

function getExtensionsGlob() {
const { extensions } = config
if (!extensions.length) {
throw new Error('You must configure at least one extension to compile in webpacker.yml')
}
return extensions.length === 1 ? `**/${extensions[0]}` : `**/*{${extensions.join(',')}}`
}

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 29, 2017

Oops sorry I meant lb before/after if block.

@javan
Copy link
Member Author

javan commented Aug 29, 2017

I prefer the spacing as-is since it's a relatively short block of code. And the linter isn't complaining. 😬

@javan javan changed the title WIP: Node module Node module Aug 29, 2017
const { existsSync } = require('fs')

function createEnvironment() {
const path = `./environments/${process.env.NODE_ENV}`

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 29, 2017 Member

@javan Missing file extension here

This comment has been minimized.

@javan

javan Aug 29, 2017 Author Member

Not needed. I removed them all throughout the codebase. Just like Ruby, Node's require doesn't need an extension.

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 29, 2017 Member

but existsSync won't work, it requires a path

const path = resolve(__dirname, `./environments/${process.env.NODE_ENV}.js`)

https://nodejs.org/api/fs.html#fs_fs_existssync_path

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 29, 2017 Member

const constructor = existsSync(path) ? require(path) : Environment

Will always evaluate to false and return default config

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

D'oh, right you are. Great catch! Fixed in df21fae

"description": "Webpacker makes it easy to use the JavaScript preprocessor and bundler [Webpack](https://webpack.github.io) to manage application-like JavaScript in Rails. It coexists with the asset pipeline, as the purpose is only to use Webpack for app-like JavaScript, not images, css, or even JavaScript Sprinkles (that all continues to live in app/assets).",
"main": "index.js",
"main": "package/index.js",

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 29, 2017 Member

Need to add files we are distributing from NPM so the package doesn't include ruby files.

  "files": [
    "package"
  ]

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

👍 e0ba006

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 30, 2017

Great, will take a final look now and we can merge

@gauravtiwari gauravtiwari force-pushed the node-module branch from b7d8aea to 3cd2e49 Aug 30, 2017
README.md Outdated
test: /.(ts|tsx)$/,
loader: 'ts-loader'
})
```

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

I removed this step because we're adding all loaders by default now, but didn't notice the tsx extension part. Maybe we should just add it to the loader?

module.exports = {
test: /\.ts$/,
loader: 'ts-loader'
}

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

@javan Oh yes, totally forgot. Yes, makes sense 👍

README.md Outdated
}
// Update an option directly
const vueLoader = environment.loaders.get('vue')
vueLoader.test = /\.vue(\.erb)?$/

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

Should we make this the default test for the vue loader?

test: /\.vue$/,

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

Yeah we can do that but I guess then we would need to update all loaders to support .erb (if we want that out of the box)?

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

We already do for ~half of the loaders. Might as well for all?

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

Sounds good 👍 We also received this through many PR's and issues so makes sense 💯

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

Cool. Want to make that change (and also add tsx to the typescript loader)?

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

on it 👍

@gauravtiwari
Copy link
Member

gauravtiwari commented Aug 30, 2017

All done 👍 added an small change to dev server to add overlay for errors - this will basically show a screen with error instead of blank screen.

README.md Outdated
@@ -90,7 +90,7 @@ in which case you may not even need the asset pipeline. This is mostly relevant

## Features

* [Webpack 2](https://webpack.js.org/)
* [Webpack 2 and 3](https://webpack.js.org/)

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

Maybe drop the 2? We technically don't support it now:

"webpack": "^3.5.5",

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

Hmm yeah, although the config we have will work with both versions but makes sense to just say 3.

Also, we should add that module concatenation plugin? https://github.com/webpack/webpack/tree/master/examples/scope-hoisting#webpackconfigjs

This comment has been minimized.

@gauravtiwari

gauravtiwari Aug 30, 2017 Member

Will push an update now 👍

This comment has been minimized.

@javan

javan Aug 30, 2017 Author Member

The config may work, but you won't be able to install v2 if you have webpacker as a dependency in package.json

gauravtiwari and others added 5 commits Aug 30, 2017
…dule

* 'node-module' of github.com:rails/webpacker:
  Fix require for scoped package
@javan

This comment has been minimized.

Copy link
Member

javan commented on 2146503 Aug 30, 2017

You can tell git to rebase on git pull to prevent merge commits like this

$ git config branch.autosetuprebase always

This comment has been minimized.

Copy link
Member Author

gauravtiwari replied Aug 30, 2017

Sure thing (forgot to add --rebase flag 😄 but this is even better)

@javan javan merged commit 50aaf77 into master Aug 30, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@javan javan deleted the node-module branch Aug 30, 2017
@olivierlacan

This comment has been minimized.

Copy link

olivierlacan commented on 012a951 Feb 28, 2018

@javan Any reason the constant declaration is different for environment.js? This causes an error when I start bin/webpack --watch --colors --progress:

04:21:36 watcher.1   | .../appname/config/webpack/environment.js:1
04:21:36 watcher.1   | (function (exports, require, module, __filename, __dirname) { const { environment } = require('@rails/webpacker')
04:21:36 watcher.1   |                                                                     ^
04:21:36 watcher.1   |
04:21:36 watcher.1   | SyntaxError: Unexpected token {
04:21:36 watcher.1   |     at exports.runInThisContext (vm.js:53:16)
04:21:36 watcher.1   |     at Module._compile (module.js:387:25)
04:21:36 watcher.1   |     at Object.Module._extensions..js (module.js:422:10)
04:21:36 watcher.1   |     at Module.load (module.js:357:32)
04:21:36 watcher.1   |     at Function.Module._load (module.js:314:12)
04:21:36 watcher.1   |     at Module.require (module.js:367:17)
04:21:36 watcher.1   |     at require (internal/module.js:16:19)
04:21:36 watcher.1   |     at Object.<anonymous> (.../appname/config/webpack/development.js:1:83)
04:21:36 watcher.1   |     at Module._compile (module.js:413:34)
04:21:36 watcher.1   |     at Object.Module._extensions..js (module.js:422:10)
04:21:37 watcher.1   | exited with code 1

This comment has been minimized.

Copy link

olivierlacan replied Feb 28, 2018

Never mind, I was booting with bash interactive shell by mistake.

I hadn't updated my .bashrc (loaded for non-login interactive shells) with a necessary load for nodenv. Instead my .bashrc was loading nvm which itself had an old pre-ES6 version of Node available. Hence the error on destructuring imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.