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

Delete destination environment file before writting new #1034

Closed
wants to merge 3 commits into from
Closed

Delete destination environment file before writting new #1034

wants to merge 3 commits into from

Conversation

lorond
Copy link
Contributor

@lorond lorond commented Feb 4, 2019

issue: #1031

@3cp
Copy link
Member

3cp commented Feb 4, 2019

@JeroenVinke I am not sure is this too much for 90% of users who do not use TFS? Is there any other option we have (like hiding this behind a custom menu)?

@avrahamcool
Copy link
Contributor

why change the target file readonly flag?
if we are about to replace the whole file one line after that.
why don't just delete the file to make sure that a new one can be safely created?

I thought you wanted to change the readonly flag on the source file while copying it to the destination - so the target will always be without readonly.

on the same note:
is that the best way to handle environment settings?
by copying a file into the src right before build?
it's annoying to have a file in the src folder - who need's to not be checked-in into source control.
is there a better to way to handle this?

@JeroenVinke
Copy link
Collaborator

@lorond thanks for looking into this and creating the pr!

I also think that there are better ways to handle these environment settings. While we look into that, people who use tfs and run into this problem can use the fix that’s in this PR.

@3cp
Copy link
Member

3cp commented Feb 6, 2019

Probably need to add an entry here https://aurelia.io/docs/cli/recipes
The repo is here https://github.com/aurelia/documentation/blob/master/current/en-us/11.%20cli/7.%20recipes.md

@lorond would you like to help on the doc?

@lorond
Copy link
Contributor Author

lorond commented Feb 7, 2019

if we are about to replace the whole file one line after that.
why don't just delete the file to make sure that a new one can be safely created?

Thought, it would fail to delete readonly file, but it isn't. Changed it.

@lorond would you like to help on the doc?

Sure

@lorond lorond changed the title Remove readonly attribute on destination environment file Delete destination environment file before writting new Feb 7, 2019
@3cp
Copy link
Member

3cp commented Feb 7, 2019

@lorond LOL, just found out gulp.dest support changing mode out of box.

gulp.src('a.js')
.pipe(gulp.dest('out', {mode: parseInt('666', 8)}));

Or if you want more strict, parseInt('644', 8).

@lorond
Copy link
Contributor Author

lorond commented Feb 7, 2019

@huochunpeng not sure if it is a good solution as it changes mode after file copied, not before.
Eventually we need to overwrite artifacts of previous build. I guess, cleanup concept would be more appropriate, and as @avrahamcool noted preliminary file removal fits better.

@3cp
Copy link
Member

3cp commented Feb 7, 2019

I tested gulp.dest thing, it works very well when source file is read only, it ensures target file is not read only.

@lorond
Copy link
Contributor Author

lorond commented Feb 7, 2019

Yep, but consider it already readonly - someone run build without solution, fount fix, integrated it and run build again - build will fail. Removing readonly is kind of hack, removed entire file - normal cleanup (and could be implemented as separate task).

@avrahamcool
Copy link
Contributor

is this PR going to be merged?

it's a beautiful solution for a common TFS problem.
it will not affect those who are not using TFS.
its far better than just pointing the user to a recipe in the docs.

@3cp
Copy link
Member

3cp commented Mar 28, 2019

@EisenbergEffect @JeroenVinke how do you think of bringing in this fix?

@lorond the new files to be touched are

skeleton/cli-bundler/aurelia_project/tasks__if_babel/transpile.js
skeleton/cli-bundler/aurelia_project/tasks__if_typescript/transpile.ts
skeleton/webpack/aurelia_project/tasks/environment.ext

@EisenbergEffect
Copy link
Contributor

I'm fine with merging this if someone can resolve the conflicts.

@lorond
Copy link
Contributor Author

lorond commented Mar 30, 2019

I'll resolve them.
@huochunpeng thanks.

@3cp
Copy link
Member

3cp commented Mar 31, 2019

Also, please rewrite commit message to follow our Git Commit Guidelines. I guess this one is a "fix(all): ..." as the bug was in both cli-bundler and webpack based projects.

Thanks for the contribution!

@lorond lorond closed this Jun 14, 2019
@lorond
Copy link
Contributor Author

lorond commented Jun 14, 2019

done #1118

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.

5 participants