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

Adds tests for environment.toWebpackConfig #1099

Merged
merged 2 commits into from
Dec 15, 2017

Conversation

rossta
Copy link
Member

@rossta rossta commented Dec 13, 2017

As a follow up to #1097 and #1098, I've taken a stab at adding tests for environment.toWebpackConfig.

Because the environment module assumes the location of config/webpacker.yml, adding tests for this module presents a challenge since this location does not exists relative to the root of the project. Possible approaches include simply adding config/webpacker.yml to the root of the project (and potentially other app files, like app/javascript/packs), generating files during the test run, or using a mock filesystem.

The approach I landed on makes it possible to customize the RAILS_ROOT location so that the environment paths resolve relative to test/test_app in tests. While approach adds flexibility, it also means ensuring that relative paths, e.g. node_modules, resolve relative to the given root if it exists.

Let me know what you think of this approach.

Adds a root module to the npm pacakge to make it possible to customize
the RAILS_ROOT location. Supporting this approach means ensuring that
relative paths for various Webpack configuration items resolve relative
to the given root if it exists.
@rossta rossta force-pushed the add-environment-package-tests branch from d4ea578 to 755174d Compare December 13, 2017 19:28
package/root.js Outdated
@@ -0,0 +1,11 @@
const { resolve, join } = require('path')
const root = process.env.RAILS_ROOT || '';
Copy link
Member

@gauravtiwari gauravtiwari Dec 13, 2017

Choose a reason for hiding this comment

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

What do you think about using process.cwd()? - fs.realpathSync(process.cwd())

@@ -77,7 +78,7 @@ const getBaseConfig = () =>
},

resolveLoader: {
modules: ['node_modules']
modules: [join(root, 'node_modules')]
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is necessary??

@rossta
Copy link
Member Author

rossta commented Dec 14, 2017

Turns out we can change the working directory of the test process, taking advantage of how we execute requires of the default and project webpacker.yml files, without changing application code.

I'm not sure how useful the original approach of introducing a configurable Rails root would be. In any case, it should probably be a separate PR rather than a side effect of trying to test existing app code.

@@ -1,4 +1,5 @@
/* global test expect */
console.log('objectify test cwd', process.cwd());
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that was a debugging line that should be removed (and looks like you've done that on master already).

@gauravtiwari
Copy link
Member

Awesome @rossta 💯 🍰

Just one minor comment and then we can merge. Thank you :)

@gauravtiwari gauravtiwari merged commit 6bccc41 into rails:master Dec 15, 2017
@rossta
Copy link
Member Author

rossta commented Dec 15, 2017

@gauravtiwari 👍

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