Fix webpack browser install #1844
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stevemk14ebr
changed the title
jaylinski
requested changes
Mar 23, 2022
package.json
Outdated
| @@ -70,12 +70,13 @@ | |||
| "uglify-js": "^3.1.4", | |||
| "underscore": "^1.5.1", | |||
| "webpack": "^1.12.6", | |||
| "webpack-dev-server": "^1.12.1" | |||
| "webpack-dev-server": "^1.12.1", | |||
| "coffeescript": "^2.6.1", | |||
There was a problem hiding this comment.
Why did you add coffeescript as a dependency? The build doesn't seem to fail...
Please provide the error output.
There was a problem hiding this comment.
~/source/repos/tmp/aster/node_modules/handlebars npm run build
> [email protected] build
> grunt build
Loading "bg-shell.coffee" tasks...ERROR
>> Error: Grunt attempted to load a .coffee file but CoffeeScript was not installed.
>> Please run `npm install --dev coffeescript` to enable loading CoffeeScript.
Loading "child-process.coffee" tasks...ERROR
>> Error: Grunt attempted to load a .coffee file but CoffeeScript was not installed.
>> Please run `npm install --dev coffeescript` to enable loading CoffeeScript.
Running "babel:cjs" (babel) task
Running "webpack:handlebars" (webpack) task
Version: webpack 1.15.0
Asset Size Chunks Chunk Names
handlebars.js 572 kB 0 [emitted] main
Running "webpack:runtime" (webpack) task
Version: webpack 1.15.0
Asset Size Chunks Chunk Names
handlebars.runtime.js 383 kB 0 [emitted] main
Done.
There was a problem hiding this comment.
I removed the dependency so this can move forward. I'm probably doing something wrong, but that's the error I received, and why I added it. Works without errors with coffeescript.
|
the big diff from that is just from json formatting ^ |
|
@stevemk14ebr @jaylinski Any update on this PR? I am hoping to be able to use handlebars with create-react-app (webpack 5 under the hood, presumably). |
|
I'll have a look at it this weekend. |
jaylinski
added a commit
that referenced
this pull request
May 15, 2022
As explained in issue #1844 and in issue webpack/webpack#15007 (comment), the way we used the `browser`-field was wrong. The main reason for using the `browser`-field is the requirement of `require('fs')` in the main-entry-file. The workaround for this was using `require('handlebars/lib/handlebars')`, but now it will also work via `require('handlebars')` for bundlers that respect the `browser`-field. The `"./runtime"`-config was removed, because it didn't have any effect. In order to detect regressions, the webpack-integration test was extended to test with different webpack versions. Fixes #1174 Closes #1844
1 task
|
I added a test for this bug and opened a new PR: #1862 I'll merge and tag it (hopefully) next week. |
jaylinski
added a commit
that referenced
this pull request
May 15, 2022
As explained in issue #1844 and in issue webpack/webpack#15007 (comment), the way we used the `browser`-field was wrong. The main reason for using the `browser`-field is the requirement of `require('fs')` in the main-entry-file. The workaround for this was using `require('handlebars/lib/handlebars')`, but now it will also work via `require('handlebars')` for bundlers that respect the `browser`-field. The `"./runtime"`-config was removed, because it didn't have any effect. In order to detect regressions, the webpack-integration test was extended to test with different webpack versions. Fixes #1174 Closes #1844
|
@jaylinski Awesome, thank you! |
jaylinski
added a commit
that referenced
this pull request
May 17, 2022
As explained in issue #1844 and in issue webpack/webpack#15007 (comment), the way we used the `browser`-field was wrong. The main reason for using the `browser`-field is the requirement of `require('fs')` in the main-entry-file. The workaround for this was using `require('handlebars/lib/handlebars')`, but now it will also work via `require('handlebars')` for bundlers that respect the `browser`-field. The `"./runtime"`-config was removed, because it didn't have any effect. In order to detect regressions, the webpack-integration test was extended to test with different webpack versions. Fixes #1174 Closes #1844
jaylinski
added a commit
that referenced
this pull request
May 17, 2022
As explained in issue #1844 and in issue webpack/webpack#15007 (comment), the way we used the `browser`-field was wrong. The main reason for using the `browser`-field is the requirement of `require('fs')` in the main-entry-file. The workaround for this was using `require('handlebars/lib/handlebars')`, but now it will also work via `require('handlebars')` for bundlers that respect the `browser`-field. The `"./runtime"`-config was removed, because it didn't have any effect. In order to detect regressions, the webpack-integration test was extended to test with different webpack versions. Fixes #1174 Closes #1844
jaylinski
added a commit
that referenced
this pull request
May 17, 2022
As explained in issue #1844 and in issue webpack/webpack#15007 (comment), the way we used the `browser`-field was wrong. The main reason for using the `browser`-field is the requirement of `require('fs')` in the main-entry-file. The workaround for this was using `require('handlebars/lib/handlebars')`, but now it will also work via `require('handlebars')` for bundlers that respect the `browser`-field. The `"./runtime"`-config was removed, because it didn't have any effect. In order to detect regressions, the webpack-integration test was extended to test with different webpack versions. Fixes #1174 Closes #1844
|
Bump on doing a release on npm? |
|
I released a new patch version (v4.7.8) on npm. Sorry it took so long. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1174
In webpack 5 things seem to have changed. From webpack/webpack#15007 which itself is from facebook/create-react-app#11756 the fs polyfills webpack used to include by default are no longer included. The browser wildcard '.' isn't valid according to webpack/webpack#15007 and should refer to a full filepath. I've changed the config to be valid, and it seems to work as far as I can tell. There was also a missing coffeescript dev dependency which caused the grunt build to fail, I added this too.