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

Switched development environment to Yarn #1345

Merged
merged 15 commits into from
Jan 10, 2019
Merged

Switched development environment to Yarn #1345

merged 15 commits into from
Jan 10, 2019

Conversation

pomek
Copy link
Member

@pomek pomek commented Nov 7, 2018

Suggested merge commit message (convention)

Other: Switched development environment to Yarn. Closes #1214.


Additional information

Side note: Indentations in .travis.yml have changed once again because I used mgit and a script (specified below) for modifying the configuration file.

#!/usr/bin/env node

const path = require( 'path' );
const fs = require( 'fs' );
const cwd = process.cwd();
const configPath = path.join( cwd, '.travis.yml' );

if ( !fs.existsSync( configPath ) ) {
	return;
}

// https://www.npmjs.com/package/js-yaml
const YAML = require( 'js-yaml' );
const config = YAML.load( fs.readFileSync( configPath, 'utf-8' ) );

config.sudo = 'required';

config.dist = 'trusty';

config.addons = {
	firefox: 'latest',
	apt: {
		sources: [ 'google-chrome' ],
		packages: [ 'google-chrome-stable' ]
	}
};

config.language = 'node_js';

config.node_js = [ '8' ];

config.cache = { yarn: true };

config.branches = {
	except: [ 'stable' ]
};

config.before_install = [
	'export DISPLAY=:99.0',
	'sh -e /etc/init.d/xvfb start',
	'npm i -g yarn'
];

config.install = [
	'yarn add @ckeditor/ckeditor5-dev-tests',
	'ckeditor5-dev-tests-install-dependencies'
];

config.script = [ 'ckeditor5-dev-tests-travis' ];
config.after_success = [ 'ckeditor5-dev-tests-save-revision' ];

fs.writeFileSync( configPath, YAML.safeDump( config, { flowLevel: -1, lineWidth: 10000} ), 'utf8' );

@pomek pomek requested a review from Reinmar November 7, 2018 11:47
@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

Why did it change yarn.lock?

image

I don't understand yet how and when yarn.lock should be updated. But I assume that calling yarn install shouldn't cause that. Am I right?

cc @ma2ciek

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

Is there a chance that changes in package.json (merged from master) caused this? I can see that the committed version of deps are old:

image

.travis.yml Outdated
script:
- npm t -- --reporter=dots
- npm run docs:api -- --validate-only
- yarn run test --reporter=dots
Copy link
Member

Choose a reason for hiding this comment

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

Can it be like this?

Suggested change
- yarn run test --reporter=dots
- yarn test --reporter=dots

Copy link
Contributor

Choose a reason for hiding this comment

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

yarn allows to do it. But IMO it might be risky to do it in .travis file.

@ma2ciek
Copy link
Contributor

ma2ciek commented Nov 22, 2018

Changing the package.json or calling yarn upgrade may change the yarn.lock. Calling yarn install shouldn't change.

.travis.yml Outdated
- npm t -- --reporter=dots
- npm run docs:api -- --validate-only
- yarn run test --reporter=dots
- yarn run docs:api --validate-only
Copy link
Member

Choose a reason for hiding this comment

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

Also no need to run.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I made a couple of suggestions here in this PR, but it'll be easier to change this automatically in all places. s/yarn run/yarn seems to work for me.

@@ -102,7 +100,7 @@ lrwxr-xr-x 1 p staff 25 31 Jan 10:37 ckeditor5-engine -> ../../../ckedito
If everything worked correctly, you should be able to run some tests:

```bash
npm run test -- --files=core
yarn run test --files=core
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yarn run test --files=core
yarn test --files=core

## Running tests

In order to run tests you need to use the `test` and `manual` tasks.

```bash
npm test -- --watch --coverage --source-map --files=engine
yarn run test --watch --coverage --source-map --files=engine
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yarn run test --watch --coverage --source-map --files=engine
yarn test --watch --coverage --source-map --files=engine

@ma2ciek
Copy link
Contributor

ma2ciek commented Nov 22, 2018

IMO using yarn <our command> is risky as we don't know if yarn won't introduce its own command under the same name in the future.

@pomek
Copy link
Member Author

pomek commented Nov 22, 2018

image

This is a reason why I propose using yarn run ...

@pomek
Copy link
Member Author

pomek commented Nov 22, 2018

Task ends with the error code but there was no error. But maybe it should be reported to Yarn.

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

OK, good points.

@Reinmar
Copy link
Member

Reinmar commented Jan 6, 2019

I miss the old good lerna clean task. Can you add yarn run clean which would do the same thing?

I think that it will boil down to executing:

rm -rf node_modules && mgit exec 'rm -rf node_modules'

@pomek
Copy link
Member Author

pomek commented Jan 7, 2019

Sounds good.

@Reinmar
Copy link
Member

Reinmar commented Jan 7, 2019

Also, we need to update this in package.json:

    "reset": "rm -rf ./packages ./node_modules && mgit bootstrap && lerna bootstrap",
    "reinstall": "lerna clean --yes && lerna bootstrap",

@Reinmar
Copy link
Member

Reinmar commented Jan 7, 2019

I left 2 minor comments in https://github.com/ckeditor/ckeditor5-dev/pull/453/files

Other than that, I'd love if we could return to the previous .travis.yml formatting. Otherwise, if we'd use travis encrypt in new packages (or existing ones) the formatting will differ.

@pomek
Copy link
Member Author

pomek commented Jan 7, 2019

Other than that, I'd love if we could return to the previous .travis.yml formatting.

Done.

@Reinmar Reinmar merged commit 26dfea4 into master Jan 10, 2019
@Reinmar Reinmar deleted the t/1214 branch January 10, 2019 10:06
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.

3 participants