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

fix: adjust module resolution behavior #1152

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

michaelsbradleyjr
Copy link
Contributor

@michaelsbradleyjr michaelsbradleyjr commented Dec 2, 2018

Include yarn's global path when modifying NODE_PATH since dependencies are deduped when a package is installed globally with yarn, which is different from npm's behavior.

Fix webpack resolution by listing relative 'node_modules' in resolve/Loader:{modules:[...]}. This ensures that dependecies' dependecies are resolved correctly when webpack builds a DApp.

Remove the invocation of .catch() on a subscription object which lacks that method.

@michaelsbradleyjr
Copy link
Contributor Author

Safari browser on macOS is unable to connect with websocket with these changes in place. The reason is unknown at this time, and it may be related to a bug/s in web3 beta.34.

Copy link
Collaborator

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Just curious. If a user had 3.2 and eject its webpack.config and then updates to 4, will his webpack be "bad"? I mean, will it break for him/her?

@michaelsbradleyjr
Copy link
Contributor Author

@jrainville yes, the user would need to re-eject and re-customize webpack.config.js. Going forward, we'll need to be careful that minor/patch versions bumps don't introduce incompatibilities for ejected webpack configs; breakage across major versions is okay, I think, but we'll need to document it somehow, e.g. in release notes.

@michaelsbradleyjr michaelsbradleyjr force-pushed the fix/module-resolution branch 2 times, most recently from 1c2a0fa to 8ac173d Compare December 3, 2018 17:40
@jrainville
Copy link
Collaborator

Ok, yeah.
We for sure need to document it.
Maybe we could even try to detect it. We could put a version inside webpack.config and if the version is too low, we tell the user to re-eject

@michaelsbradleyjr
Copy link
Contributor Author

Good idea!

Include yarn's global path when modifying `NODE_PATH` since dependencies are
deduped when a package is installed globally with yarn, which is different from
npm's behavior.

Fix webpack resolution by listing relative `'node_modules'` in
`resolve/Loader:{modules:[...]}`. This ensures that dependecies' dependecies
are resolved correctly when webpack builds a DApp.

Remove the invocation of `.catch()` on a subscription object which lacks that
method.
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.

4 participants