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

Some fixes #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Some fixes #13

wants to merge 5 commits into from

Conversation

singggum3b
Copy link

Hi , i add "main:src" for development with webpack & fixed a bug with dispatch not being passed down more than 1 level.

@mattkrick
Copy link
Owner

thank you! that main:src is interesting, google doesn't index on colons so I can't find any info on it, could you send me a link about it?

@singggum3b
Copy link
Author

Webpack has a feature to let you set package main field https://webpack.github.io/docs/configuration.html#resolve-packagemains

@singggum3b
Copy link
Author

The safari bug is actually not fixed yet.I'm working on it, & can test it tomorrow, i dont have a mac here

@mattkrick
Copy link
Owner

mattkrick commented Sep 13, 2016

ah, so the user would still have to define a packageMains in their webpack in order to get to the source?

Personally, I'd prefer to accomplish this in 1 of 2 ways:

  • in your webpack config, include npm_modules/redux-operations... in the list of things you babelfy
  • Use symlinks/watch
    • in terminal 1, open redux-operations. run npm link, then npm run watch
    • In terminal 2 (your project) run npm link redux-operations

The 2nd solution is the one i always use, primarily because I like to keep projects in separate windows.

@singggum3b
Copy link
Author

Yeah, it's kind of just another option. I don't think it's harmful. But i had bitterness experience with npm link. 😭 .
My current approach is a bit different, i just have to create a sub_module folder, add it to git ignore, then clone the src into that folder, set it to higher priority in webpack.
If you have webstorm it can also detect multiple git root for you

@mattkrick
Copy link
Owner

What went wrong with npm link? I know webpack doesn't play well with it, but that's why i run the watch so it compiles pre-webpack build. Copying the src into a folder that is inside the project root burned me before due to an instanceof problem. If it's inside your project, you're guaranteed that it'll be the same constructor function, but then once you break it into its own package, that guarantee goes away leaving you with really difficult errors.

@singggum3b
Copy link
Author

I'm not sure i understand the instanceof problem, since i've not encountered it yet . But for npm link, it broke everytime i update or install dependency & manually adding dir inside node_modules to webpack is not really refreshing.
About the safari bug, i tested it, & look like it worked.
So do you want me to remove the "main:src" ?
Can we merge this now?

@mattkrick
Copy link
Owner

hey @singggum3b sorry for the late reply, just getting back from a little vacation.
The instanceof problem is one where you have 2 separate instances of the same constructor function, so a instanceof Foo isn't always right, depending on which one instantiated it.

The symlink shouldn't break when you install npm packages, only if you remove the symlink reference from your node_modules will it ever break (eg rimraf your node_modules). I'd love to dig into this & reproduce if possible!

removing main:src & changing the tabs to spaces (i should probably put an eslint on this thing) should make this good to merge!

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