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

add onlyIfDevelopment entry file for easier webpack config #9

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

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Aug 10, 2018

  • add documentation for this option and leave the previous installation
    docs as an alternative "custom installation"

I based the docs off of the previous commit that mentioned entry files, with a few modifications:

  • Added a link to webpack entry config since it's a bit of an advanced topic
  • Removed require.resolve as I don't believe it's necessary (I didn't need to use it in my webpack config, though I'm not sure if there's a circumstance in which it would be necessary)

Sorry, my formatter removed whitespace at the end of lines automatically, I can remove those from the commit if needed.

Having looked at the past docs commit, I can see that my usage is a bit different than the way you've used it / seen it. I didn't add a new entry (and therefore no new <script> tag), I added it to the array in my entry, i.e.

entry: {
  main: ['webpack-serve-overlay/onlyIfProduction', './main.js']
}

See the link above for full context in my boilerplate repo.
EDIT: I definitely think having it as a separate / new entry is the ideal usage as separate entrypoints / script tags ensures the overlay always shows up as its separate, isolated bundle will never fail to compile. In that case, excluding the overlay script tag from production HTML would also be ideal to have as little production impact as possible. Even with onlyIfProduction, webpack's bundling code will still create a tiny bundle of JS that does nothing (not optimal, but not particularly harmful either).

Related to #5

Copy link
Owner

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Sorry it took a couple of days for me to get around to looking this over!

I'll give it a quick test with using entry, but aside from the two requested changes, it's looking pretty good!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@G-Rath
Copy link
Owner

G-Rath commented Aug 12, 2018

Removed require.resolve as I don't believe it's necessary

I think this was based off the use of path.resolve with a lot of the other path-related things in webpack.

Having not had much experience with entry, I can't say if it's needed or not - it could very well have been the whole reason I was having problems.

I'm happy to defer to you for advise on the best way to handle the entry side of things 😃

README.md Show resolved Hide resolved
@agilgur5 agilgur5 force-pushed the only-if-production branch from 1c93ee4 to a001383 Compare August 14, 2018 04:42
- add documentation for this option and leave the previous installation
  docs as an alternative "custom installation"
@agilgur5 agilgur5 force-pushed the only-if-production branch from a001383 to fe8c5cb Compare August 14, 2018 04:43
@agilgur5
Copy link
Author

Committed a new revision that should change it to onlyIfDevelopment and remove the formatter changes

@agilgur5 agilgur5 changed the title add onlyIfProduction entry file for easier webpack config add onlyIfDevelopment entry file for easier webpack config Aug 14, 2018
- as doing so will break hot reloading for the default settings
- add link to the relevant webpack-serve issue & comment
@agilgur5
Copy link
Author

agilgur5 commented Sep 2, 2018

Added a new commit with an addition to the docs regarding the hot reloading problem that occurs when this is the first entry

- should make the hot reload problem harder to stumble upon by making
  the intended entry config more explicit
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