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

"p-*"-files are not immutable. Hash-filename doesn't update when content changes. #2526

Open
mikkelrom opened this issue Jun 22, 2020 · 26 comments
Labels
Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team

Comments

@mikkelrom
Copy link

mikkelrom commented Jun 22, 2020

Stencil version:

 @stencil/core@1.12.2

I'm submitting a:

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://stencil-worldwide.herokuapp.com/ or https://forum.ionicframework.com/

Current behavior:

Stencil-config setting: hashFileNames not working entirely as expected. p-*-files are not immutable even though they should be, since the content within can change without the filename itself changes.

Expected behavior:

p-*-filenames should change if the content within changes, so those files can be "forever-cached", which they cannot at the moment even though the docs says:

During production builds, the content of each generated file is hashed to represent the content, and the hashed value is used as the filename. If the content isn't updated between builds, then it receives the same filename. When the content is updated, then the filename is different. By doing this, deployed apps can "forever-cache" the build directory and take full advantage of content delivery networks (CDNs) and heavily caching files for faster apps."

Steps to reproduce:

https://github.com/mikkelrom/stencil-caching-issue

Other information:

Related issue: #1983

Information about rollup issue

The team behind http://web.dev created this to keep track of bundlers and their errors. This failing test in Rollup seems to maybe be causing this issue: https://bundlers.tooling.report/hashing/asset-cascade/
Rollup issue: rollup/rollup#3415

@mikkelrom
Copy link
Author

Update: It looks like Vite has made a workaround for the Rollup issue, maybe this could be implemented in Stencil as well?

vitejs/vite@f8e4eeb

@splitinfinities
Copy link
Contributor

Hey @mikkelrom - I tagged this so Ryan can confirm this bug and go from there.

@mikkelrom
Copy link
Author

Thank you @splitinfinities, sounds great 🙌 🙂

@janarvaez
Copy link

Hi, is there an update on this? I'm running into the same issue.

Basically, my build produces my-library.js and a bunch of p-.system.js . When I deploy this, everything works fine.
On a second build and deploy, my-library.js calls te same file, say p-123.system.js like the last time, but p-123 has now changed and calls different p-files. Problem is, p-123 is cached and keeps calling files that don't exist anymore. Is there a way to force it so that all filenames change on every build?

@janarvaez
Copy link

I even tried setting the hashedFileNameLength to 12, and some files stay at 8 and some at 12.

@mikkelrom
Copy link
Author

@splitinfinities @rwaskiewicz any update on this? 🙂

@cjorasch
Copy link

This seems like a potentially huge foundational issue for Stencil (and Ionic) and it is a little shocking to see so little official response for something that has been lingering since being reported in 2019.

I know you guys are busy and it is annoying to get lots of issues and feature requests so it is easy for things to get lost in the list. But this one seems important. If you can not reproduce the problem, don't think it is a bug, are waiting on some other fix, have a suggested workaround, or even just agree that this is an important thing to fix that would be great to know.

There are cases where getting the wrong version of a file can have a minor impact (like stale content) but there are also cases where the wrong version of a file can totally break an application. For example, missing import files break <ion-nav> (ionic-team/ionic-framework#21046). Further details on the impact of missing files is also in #2376. Workarounds like refreshing the page, CDN expirations, etc. do not necessarily fix the problem and it seems like something that can only be handled during the build process.

If it would be helpful for me to provide any more details about the downstream impact of non-immutable files then let me know and I can provide more detail.

It is similar to if npm install sometimes resulted in incorrect versions of some of the files.

@johnjenkins
Copy link
Contributor

johnjenkins commented Feb 11, 2022

hey @mikkelrom - I took the liberty of taking your reproduction repo for a spin, but could not recreate the issue using the steps described:
"Hello world!"
hello world!
"Hello world 2!"
hello world 2!

Further to this, I created a sandbox which isolates the snippet stencil is using under the hood for file hashes here and stencil seems to be consistent...
If someone can create a reliable reproduction, I'll take a look.

Since the file hashing is done by node's internal 'crypto' module, it could boil down to the version of node used? ... I realise I'm just grasping

@mikkelrom
Copy link
Author

mikkelrom commented Feb 11, 2022

Hi @johnjenkins.
Thanks for taking the time to look at this (to me, critical) issue and the reproduction repo 👍 🙏
I've just pulled down the repo again and followed the steps, and I still get the same behaviour as described in the issue. I'm using node version 12.22.1.

As mentioned, I'm pretty convinced that the issue is caused by Rollup. Please see here: https://bundlers.tooling.report/hashing/asset-cascade/ and here: rollup/rollup#3415

And as I mentioned in the first comment:

It looks like Vite has made a workaround for the Rollup issue, maybe this could be implemented in Stencil as well? vitejs/vite@f8e4eeb

@johnjenkins
Copy link
Contributor

johnjenkins commented Feb 11, 2022

It's not clear to me how https://bundlers.tooling.report/hashing/asset-cascade/ is relevant because in this instance it's the actual 'core' file who's source is changing, not an imported asset.

And I also don't think it's to do with rollup because rollup actually doesn't control the file hashing - that step happens after the rollup bundle, (it happens here < using 'rollupResults' above) just before the files are written to disk.

Interestingly the vite fix you note here
Is very similar to what stencil is already doing here < except using sha256 instead of sha1.

So I suppose it comes down to local environment.
Using stencil v2.13, what do you get when running stencil info?

Also, have you tried enableCache: false, in your stencil.config?

@mikkelrom
Copy link
Author

Hi again @johnjenkins, sorry for the late reply.

This issue was created a long time ago, so I decided to try and update the reproduction repo to the latest version of Stencil, as you suggested (from v1.12.2 to v2.13.0). And after updating I can't seem to reproduce the issue, which is really great! 🎉 The filename seem to change when the contents of the file is changed, which is great 🙂

I'm curious to know if this issue is also solved for others, just by using latest Stencil version? @janarvaez @cjorasch ?

@janarvaez
Copy link

Hi again @johnjenkins, sorry for the late reply.

This issue was created a long time ago, so I decided to try and update the reproduction repo to the latest version of Stencil, as you suggested (from v1.12.2 to v2.13.0). And after updating I can't seem to reproduce the issue, which is really great! 🎉 The filename seem to change when the contents of the file is changed, which is great 🙂

I'm curious to know if this issue is also solved for others, just by using latest Stencil version? @janarvaez @cjorasch ?

Hi! I'm afraid I had recently upgraded at the time of my comment, which was 2.12.1, and was still encountering the issue.
Can't share my repo as it's a company owned project. I'll do my best to try and create a minimal version that reproduces this.

@janarvaez
Copy link

Hi @johnjenkins (cc @mikkelrom )

Here is a reproduction repo which can hopefully help:

https://github.com/janarvaez/stencil-build-issue

I left what I did in the README.md
This one is using 2.12.1, and I tried to remove as much code as possible, so please forgive me if there's some noise or undesired code/files in there... I also forgot to gitignore the www and loader folders 😬

@bzolotukhyn
Copy link

Hi All! Just updated my project to stencil 2.14.0 and issue still there.

% stencil info
System: node 14.17.1
Plaform: darwin (19.6.0)
Compiler: .../node_modules/@stencil/core/compiler/stencil.js
Stencil: 2.14.0 💫
TypeScript: 4.5.4
Rollup: 2.42.3
Parse5: 6.0.1
Sizzle: 2.42.3
Terser: 5.6.1

@johnjenkins
Copy link
Contributor

johnjenkins commented Feb 17, 2022

to be clear, @bzolotukhyn @cjorasch @janarvaez is the issue primarily with the .system.js modules (i.e. the es5 build)?
Or do you see it other places too?

@bzolotukhyn
Copy link

@johnjenkins I compared the last 5 builds of my library and in my case only one file (the same) doesn't change name when content changes and yes it's .system.js file.

@rwaskiewicz rwaskiewicz added Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team and removed Bug: Needs Validation labels Mar 25, 2022
@mikkelrom
Copy link
Author

Update: Rollup issue fixed in v3.0.0.

rollup/rollup#3415

@rwaskiewicz
Copy link
Member

Excellent - we're scheduling Rollup-related updates starting next sprint.

@durgaprasad95
Copy link

Excellent - we're scheduling Rollup-related updates starting next sprint.

@rwaskiewicz any update on this and process on how to integrate it with existing stencil project?

@alicewriteswrongs
Copy link
Contributor

Hey @durgaprasad95, just a quick update: we'll be sure to ping here when we have a pre-release version of Stencil with Rollup v3 to test and confirm that this issue is addressed.

Unfortunately upgrading Rollup in Stencil is quite involved because Stencil integrates very closely with Rollup, uses a lot of custom plugins, and so on. Because we're currently using a fairly outdated version we have to do something of a stepwise upgrade to eventually get up to v3.

All that is to say that while we can't make a promise as to a specific timeline at present, the team is working on it and it is a priority for us.

@john-traas
Copy link

@alicewriteswrongs is there any update on this issue?

@chiragajmeri
Copy link

Hi @alicewriteswrongs and @rwaskiewicz

Thanks for being an active participant in the StencilJS. Wondering if there are any updates for this issue?

@rwaskiewicz
Copy link
Member

Hey folks, we don't have any updates on this at this time. We put the rollup changes on hold for a few months, and will plan on picking them up again before the end of this year

@john-traas
Copy link

Thank you for the update @rwaskiewicz - much appreciated.

@jespejoh
Copy link

I came up with a workaround, in case it helps anybody: to include a build timestamp prop in all components, which forces StencilJS to re-generate all files as the content always changes with every production build. Not ideal, but given the fact that the issue has been open for 4 years I don't expect any updates on it soon.

As an idea to solve the issue in the core, instead of going down the rabbit hole of upgrading Rollup: this could be easily solved if StencilJS offered a method to pass a "?v" query string to the lazy-loader, in the same way that we set the setNonce() when loading the components. That way we ensure that all lazy-loaded files are appended with a version string on the URL, which will ensure the CDN always fetches the requested version of the file.

@akshaybachhav
Copy link

Insufficient rollup version: "@rollup/plugin-commonjs" requires at least rollup@2.68.0 but found rollup@2.56.3. (plugin: commonjs, buildStart)

i am getting this issue
below are my versions

yarn list --pattern rollup
yarn list v1.22.22
├─ @rollup/plugin-commonjs@26.0.1
├─ @rollup/pluginutils@5.1.0
├─ rollup-plugin-inject@3.0.2
├─ rollup-plugin-node-polyfills@0.2.1
├─ rollup-pluginutils@2.8.2
└─ rollup@2.68.0
Done in 0.28s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team
Projects
None yet
Development

No branches or pull requests