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

mem-fs-editor#commit callback or similar hook #1273

Closed
ozum opened this issue Feb 2, 2021 · 22 comments
Closed

mem-fs-editor#commit callback or similar hook #1273

ozum opened this issue Feb 2, 2021 · 22 comments
Labels

Comments

@ozum
Copy link

ozum commented Feb 2, 2021

Hi,

TLDR;

My generator creates and updates files. I'm providing an uninstall method to undo changes made by my generator. I need to know whether each conflicted file is written to disk or rejected by the user.

AFAIK, currently, it is not possible. It would be very nice to have a hook, callback, etc. to get informed whether a conflicted file is written to disk or not. I'm sorry if it is already possible and I missed it. Any help is appreciated.

Details

First, I thought that I can use the commit method's callback to get whether a conflict is accepted or rejected, but it is not documented in Yeoman API docs. I assumed it is an undocumented feature and should be avoided and also read the following in docs:

It is worth noting that although this.fs exposes commit, you should not call it in your generator. Yeoman calls this internally after the conflicts stage of the run loop.

Now, I am doing it in a very bad way. I read files at the beginning and reread them at the end to compare and decide whether they are accepted or rejected. Of course, this destroys all advantages of the mem-fs memory file system.

Many thanks,

@ozum ozum added the needs triage Awaiting triage label Feb 2, 2021
@tonyhallett
Copy link
Contributor

tonyhallett commented Feb 4, 2021

I think that the best option is to monkey patch fs.commit
This and an alternative solution is below.

const Generator = require('yeoman-generator');
const helpers = require('yeoman-test');
const fs = require('fs')
const path = require('path');
const through = require('through2')

describe('https://github.com/yeoman/generator/issues/1273', () =>{
    let haveConflict;
    let conflictGenerator;
    beforeEach(()=>{
      haveConflict = undefined;
    })
    const conflictFileName = 'conflict.js';
    const conflictingOriginalText = '//original';

    const ConflictGeneratorBase = class extends Generator{
      newFile(){
        this.fs.write(this.destinationPath('new.js'), '');
      }
      conflictWrite(){
        const fileText = haveConflict ? '//new text' : conflictingOriginalText;
        this.fs.write(this.destinationPath(conflictFileName), fileText);
      }
    }

    
    async function runWithConflict(conflict,skip,Generator){
      haveConflict = conflict;
      //override that set by test helpers
      const runContext = helpers.create(Generator).withOptions({force:false});
      
      if(conflict){
        runContext.withPrompts({
          action:skip?'skip':'force'
        });
      }
      runContext.doInDir(destinationDirectory => {
        const conflictFilePath = path.join(destinationDirectory,'conflict.js');
        fs.writeFileSync(conflictFilePath,conflictingOriginalText)
      })
      await runContext.run();
      conflictGenerator = runContext.generator;
    }

    
    describe('should be possible with monkey patching checkForCollision', () => {
      
      const ConflictGeneratorPatchConflicter = class extends ConflictGeneratorBase {
        fileStatuses = [];
        constructor(args, opts) {
          super(args, opts);
          const self = this;
          const checkForCollision = this.conflicter.checkForCollision;
          this.conflicter.checkForCollision = function(file,cb){
            const callback = (err, status) => {
              self.fileStatuses.push({file, status});
              cb(err,status);
            }
            checkForCollision.call(self.conflicter,file,callback);
          }
        }
        newFile(){
          super.newFile();
        }
        conflictWrite(){
          super.conflictWrite();
        }
      };
  
      async function runWithConflictExpectStatuses(conflict,skip,expectedStatus1,expectedStatus2){
        await runWithConflict(conflict,skip,ConflictGeneratorPatchConflicter);
  
        expect(conflictGenerator.fileStatuses.length).toBe(2);
        expect(conflictGenerator.fileStatuses[0].status).toBe(expectedStatus1);
        expect(conflictGenerator.fileStatuses[1].status).toBe(expectedStatus2);
      }

      [{
        conflict:true,
        skip:false,
        status1:'create',
        status2:'force',
        description:'conflict force'
      },{
        conflict:true,
        skip:true,
        status1:'create',
        status2:'skip',
        description:'conflict skip'
      },
      {
        conflict:false,
        skip:undefined,
        status1:'create',
        status2:'identical',
        description:'no conflict'
      }
      ].forEach(details => {
        it(details.description, () => {
          return runWithConflictExpectStatuses(details.conflict,details.skip,details.status1, details.status2);
        })
      })
    })

    describe('should be possible with monkey patching fs commit', () => {
      
      const ConflictGeneratorPatchCommit = class extends ConflictGeneratorBase {
        files = [];
        constructor(args, opts) {
          super(args, opts);
          const self = this;
          const conflictChecker = through.obj(function(file, enc, cb) {
            self.files.push(file);
            this.push(file);
            cb();
          });
          const commit = this.fs.commit;
          this.fs.commit = (transforms, cb) => {
            const allTransforms = [...transforms,conflictChecker];
            commit.call(this.fs,allTransforms,cb)
          }
        }
        
        newFile(){
          super.newFile();
        }
        conflictWrite(){
          super.conflictWrite();
        }
      }

      async function runWithConflictAndExpectWrites(conflict,skip,expectedNumWrites){
        await runWithConflict(conflict,skip,ConflictGeneratorPatchCommit);
        expect(conflictGenerator.files.length).toBe(expectedNumWrites);
      }

      [
        {
          description:'conflict skip',
          conflict:true,
          skip:true,
          expectedNumWrites:1
        },
        {
          description:'conflict force',
          conflict:true,
          skip:false,
          expectedNumWrites:2
        }
      ].forEach(details => {
        it(details.description, () => {
          return runWithConflictAndExpectWrites(details.conflict,details.skip,details.expectedNumWrites);
        })
      })
    })
  })

@mshima
Copy link
Member

mshima commented Feb 4, 2021

You can try this (after commit priority: install or end):

const file = this.env.sharedFs.get(this.destinationPath('foo.txt'));
if (file.committed) {
  // Written to disk
}
const file = this.env.sharedFs.each(file => {
  if (file.committed) {
    file.path;
    // Written to disk
  }
});

@ozum
Copy link
Author

ozum commented Feb 4, 2021

Many thanks, @tonyhallett, and @mshima. I will try solutions offered.

@tonyhallett
Copy link
Contributor

@ozum I'd go with @mshima solution !

@ozum
Copy link
Author

ozum commented Feb 4, 2021

@ozum I'd go with @mshima solution !

Thanks for the suggestion, I'll stick with it.

@mshima
Copy link
Member

mshima commented Feb 4, 2021

Maybe you need to pass skip-regenerate option:
Change the generator constructor passing {skipRegenerate: true} to Generator class.
Otherwise even non conflicting files will be committed.

force option commits every file without checking for conflicts.

@ozum
Copy link
Author

ozum commented Feb 4, 2021

@mshima, what does skipRegenerate does? I couldn't find it in docs.

Should I use { skipRegenerate: true } to detect written files when --force option used?

@mshima
Copy link
Member

mshima commented Feb 4, 2021

By default an identical file will be written to file, so the above suggestion will not work.
Looks like it's missing in the documentation.

this.skipRegenerate = options.skipRegenerate;

@ozum
Copy link
Author

ozum commented Feb 4, 2021

so the above suggestion will not work.

Is this.env.fs.each(file => if (file.comitted) {}) still valid?

@mshima
Copy link
Member

mshima commented Feb 4, 2021

Works when you create your generator like:

constructor(args, opts) {
    super(args, {opts, skipRegenerate: true});

@ozum
Copy link
Author

ozum commented Feb 4, 2021

@mshima, is the following correct?

{ skipRegenerate: false }: If the file exists and is identical, it is still overwritten with the same content. So, the last modified time of the file is changed.

{ skipRegenerate: true }: If the file exists and is identical, it is not overwritten. So, the last modified time of the file is not changed.

@mshima
Copy link
Member

mshima commented Feb 4, 2021

@ozum exactly

@ozum
Copy link
Author

ozum commented Feb 4, 2021

@mshima, I got this.env.fs.each is "undefined" error.

@mshima
Copy link
Member

mshima commented Feb 4, 2021

My bad, it’s sharedFs. Updated the example.

@ozum
Copy link
Author

ozum commented Feb 4, 2021

@mshima, thanks for the responses and your time.

Just a typo: file.commited should be file.committed. Maybe you want to edit your example for future readers.

I would be grateful if you help one more thing. Since this.env.sharedFs.get is shared by all generators, I should execute it at the end priority of the last generator. Assuming my generators are async, is there a way to execute my method after all generators are finished their jobs?

@tonyhallett
Copy link
Contributor

@ozum
this.fs.store would have worked from the generator.

The environment is an event emitter

This might be what you are looking for

// If runLoop has ended, the environment has ended too.
      this.runLoop.on('end', () => {
        resolve();
        this.emit('end');
      });

@mshima mshima added question and removed needs triage Awaiting triage labels Feb 4, 2021
@ozum
Copy link
Author

ozum commented Feb 4, 2021

Thanks, @tonyhallett. Could you please give more details?

Where to put the code below and what is the difference between end method and this?

 this.runLoop.on('end', () => {
        resolve();
        this.emit('end');
      });

@tonyhallett
Copy link
Contributor

Sorry this is the code from environment. You should listen for the end event on this.env

@tonyhallett
Copy link
Contributor

I think it is this.env.on('end',CB)

@tonyhallett
Copy link
Contributor

Another idea. Add a custom queue that runs after end.

@ozum
Copy link
Author

ozum commented Feb 4, 2021

Thanks @tonyhallett for the suggestions.

@tonyhallett
Copy link
Contributor

Options.customPriorities is an array. Object with priorityName that matches the method name that does the work. The queue will be added after end.

@mshima mshima closed this as completed Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants