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

Output asset permissions #211

Merged
merged 4 commits into from
Jan 9, 2019
Merged

Output asset permissions #211

merged 4 commits into from
Jan 9, 2019

Conversation

guybedford
Copy link
Contributor

This stores the permissions of assets in a side table and ensures the corresponding output asset permissions.

In theory this is a major change as the assets API is changed to be an object of { [assetName]: { source, permissions } } instead of { [assetName]: source } now.

@sokra an interesting thing here was it turned out unnecessary to maintain the cache as for some reason it seems like Webpack isn't actually caching the sources with emitted assets. Ideally these should be cached though I think?

I've left the cache persistence of asset permissions commented out for now given the above.

@codecov-io
Copy link

Codecov Report

Merging #211 into master will increase coverage by 0.81%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   67.87%   68.68%   +0.81%     
==========================================
  Files          12       12              
  Lines         579      594      +15     
==========================================
+ Hits          393      408      +15     
  Misses        186      186
Impacted Files Coverage Δ
src/cli.js 0% <0%> (ø) ⬆️
src/utils/sharedlib-emit.js 77.77% <100%> (+2.77%) ⬆️
src/loaders/node-loader.js 100% <100%> (ø) ⬆️
src/loaders/relocate-loader.js 93.12% <100%> (+0.1%) ⬆️
src/index.js 72.09% <100%> (+1.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c813b07...98494d6. Read the comment docs.

const nodeLoader = require(__dirname + "/loaders/node-loader.js");
const relocateLoader = require(__dirname + "/loaders/relocate-loader.js");
const nodeLoader = eval('require(__dirname + "/loaders/node-loader.js")');
const relocateLoader = eval('require(__dirname + "/loaders/relocate-loader.js")');
Copy link
Member

Choose a reason for hiding this comment

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

Why not require("./loaders/node-loader.js") and define it as external

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried this and it doesn't work (get Cannot read "assets" of undefined indicating there may be separate instances in play, or something else).

@rauchg rauchg merged commit ae96fbb into master Jan 9, 2019
@rauchg rauchg deleted the asset-permissions branch January 9, 2019 17:49
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