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 assets source filenames #125

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

batistadasilva
Copy link
Contributor

@batistadasilva batistadasilva commented Mar 29, 2024

Add sourceFilename field

Asset resources in webpack-stats.json file will look like,

"assets": {
  "assets/test-bbf3c94e2a3948c98900.txt": {
    "name": "assets/test-bbf3c94e2a3948c98900.txt",
    "path": "/home/user/project-root/assets/test-bbf3c94e2a3948c98900.txt",
    "publicPath": "http://localhost:3000/assets/test-bbf3c94e2a3948c98900.txt",
    "sourceFilename": "src/test.txt"
  }
}

Related to django-webpack/django-webpack-loader#343

batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Mar 29, 2024
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
lib/index.js Show resolved Hide resolved
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but please check suggested improvement.

@batistadasilva
Copy link
Contributor Author

@fjsj Thanks for the review

batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Apr 2, 2024
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Apr 2, 2024
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
fjsj pushed a commit to django-webpack/django-webpack-loader that referenced this pull request Apr 2, 2024
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes #343

Depends on django-webpack/webpack-bundle-tracker#125
lib/index.js Outdated Show resolved Hide resolved
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Please check comment.

@batistadasilva batistadasilva requested a review from fjsj April 2, 2024 13:09
@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

Some tests are breaking @batistadasilva
Could you please check?

@batistadasilva
Copy link
Contributor Author

@fjsj It seems like there is a broken test in master branch

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

The last build from 3 months ago did pass: https://github.com/django-webpack/webpack-bundle-tracker/actions/runs/7466611716
Are you seeing the same errors in localhost as well? Or in your fork?

@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 2, 2024

I am seeing the same errors when running in localhost on the commit 97c7ec6

How can I help?

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

The error is this:

    -   "js/862.js",
    +   "js/75.js",

Please check if this is simply a hash change. If so, you can safely change the test assertion.

@batistadasilva
Copy link
Contributor Author

Please check if this is simply a hash change.

Yes it was

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

@batistadasilva I think now it breaks in Webpack 4? I think it's safe to ignore sourceFilename in Webpack 4. Whoever needs to use this should upgrade, as it's not a critical feature.

@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024

@fjsj It seems like a typing error, but I don't know how to handle it

Could you help me wit this?

@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

If assetsInfo is undefined, you can ignore adding sourceFilename. Like:

if (stats.compilation.assetsInfo) {
  fileInfo.sourceFilename = stats.compilation.assetsInfo.get(assetName).sourceFilename;
}

@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024

@fjsj Done

However, the typing errors may still remain

@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

@batistadasilva I see that's a tsc error, but we can ignore it on Webpack 4.

Please enable me to edit this PR:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Or look for a way to ignore the error TS2339 in those two lines.

Asset resources in `webpack-stats.json` file will look like,

```json
"assets": {
  "assets/test-bbf3c94e2a3948c98900.txt": {
    "name": "assets/test-bbf3c94e2a3948c98900.txt",
    "path": "/home/user/project-root/assets/test-bbf3c94e2a3948c98900.txt",
    "publicPath": "http://localhost:3000/assets/test-bbf3c94e2a3948c98900.txt",
    "sourceFilename": "src/test.txt"
  }
}
```

Related to django-webpack/django-webpack-loader#343
@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024

Please enable me to edit this PR:

It would be with pleasure, but I couldn't find the checkbox the documentation is talking about

Or look for a way to ignore the error TS2339 in those two lines.

I don't think we can ignore a specific TS rule, so I ignored TS in both lines.
I hope it's fine.

@fjsj fjsj merged commit b0a7c27 into django-webpack:master Apr 3, 2024
3 checks passed
@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

Thanks! We will release soon.

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.

None yet

2 participants