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

Ability to use ES6: rollup+babel transpilation #309

Merged
merged 3 commits into from Sep 12, 2020
Merged

Conversation

drzraf
Copy link
Collaborator

@drzraf drzraf commented Sep 3, 2020

This change gives the opportunity to use ES6 for src/flow.js

  • karma.conf.js : Test the transpiled bundle instead of ES6 code.
  • babel: use bable-env + browserlist to transpile for a wider range of browsers.
  • For that purpose, switch to rollup with way handier and more modern than browserify.
  • In order to keep the toolchain backward-compatible (and reduce the PR surface), use grunt:exec to wrap rollup execution.

Also update to karma-coverage v2 needed for async support as discussed and anticipated in #304.

Raphaël Droz added 2 commits September 3, 2020 14:16
- babel: use bable-env + browserlist to transpile for a wider range of browsers.
- For that purpose, switch to `rollup` with way handier and more modern than browserify.
- In order to keep the toolchain backward-compatible (and reduce the MR surface), use `grunt:exec` to wrap `rollup` execution.
@AidasK
Copy link
Member

AidasK commented Sep 4, 2020

You are doing a huge upgrade and that's great. Would you like to be added as a maintainer of this project?

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 4, 2020

As long as someone keeps reviewing PR, I've no problem with this but I can't guarantee long-term involvement.

Still, about that specific PR, I don't understand why the CI fails, is this something you could fix to get this green?

@AidasK
Copy link
Member

AidasK commented Sep 5, 2020

It looks like it can't generate karma coverage report because coverage plugin is missing. Though all the tests pass.
You can see the log here: https://travis-ci.org/github/flowjs/flow.js/jobs/723863503

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 7, 2020

I guess the plugin is not loaded (anymore) because node can't parse node_modules/karma-sauce-launcher. Updating the version of node in .travis.yml may do it?

@AidasK
Copy link
Member

AidasK commented Sep 8, 2020

https://travis-ci.org/github/flowjs/flow.js/jobs/724987732
now we are getting different error

/home/travis/.nvm/versions/node/v14.9.0/lib/node_modules/codeclimate-test-reporter/formatter.js:33
      throw parseError;
      ^
Unknown input coverage format
(Use `node --trace-uncaught ...` to show where the exception was thrown)
The command "$TRAVIS_BUILD_DIR/travis.sh" exited with 1.

It looks like coverage was created, but with different format or empty file.

@AidasK
Copy link
Member

AidasK commented Sep 8, 2020

can you test if grunt karma:coverage generates a valid coverage report? e.g. it should create coverage/*/lcov.info files

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 8, 2020

Could you set your CC_TEST_REPORTER_ID if different from CODECLIMATE_REPO_TOKEN?
Note: These variables (like SAUCE_ACCESS_KEY) should probably be secret and stored inside GitHub configuration.
You may also want to remove some configuration from the browsers sauce test against.

It's not doing coverage reporting (0-size file). But I don't understand which tool is actually in charge of code instrumentation?

@AidasK
Copy link
Member

AidasK commented Sep 9, 2020

Great job, it works! I have added you to the maintainers of this project. Once finished, your branch will be released as v3.0.0 flowjs since it supports promises. Please don't forget documentation too 🥇

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 9, 2020

Does not work. lcov.info is 0 bytes.

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 9, 2020

code-coverage fixed.
Should I let you squash-merge on -master? Or on a v3 branch if you prefer such solution the time being?

@drzraf drzraf requested a review from AidasK September 9, 2020 14:33
@AidasK
Copy link
Member

AidasK commented Sep 10, 2020

Yes please, squash it and push it to v3 branch. Feel free to do it yourself, you have all the permissions you need. We need some time to release it to master once it's tested.

@drzraf drzraf changed the base branch from master to v3 September 12, 2020 02:43
@drzraf drzraf merged commit cfcc849 into flowjs:v3 Sep 12, 2020
@drzraf
Copy link
Collaborator Author

drzraf commented Sep 16, 2020

Note: The update to karma-coverage v2 caused start --single-run=false to stop functioning.
(karma-runner/karma-sauce-launcher#217)
Will fix this in a subsequent commit.

drzraf pushed a commit to drzraf/flow.js that referenced this pull request Sep 16, 2020
…h happen

 to have a bug when `--single-run=false` is used.

This commit:
- move more of the static Karma configuration from Gruntfile back into karma.conf
- add two SauceLabs browsers flavors
- add a guard when karma:saucelabs is used without defined SauceLabs credentials
drzraf pushed a commit to drzraf/flow.js that referenced this pull request Sep 16, 2020
In flowjs#314 (a0eafbc) we updated karma-sauce-launcher from v0 to v4
 but v2 introduced a bug keeping `--single-run=false` from working.

This commit:
- moves more of the static Karma configuration from Gruntfile back into karma.conf
- adds two SauceLabs browsers flavors
- adds a guard when karma:saucelabs is used without defined SauceLabs credentials
- disables the saucelabs in the default karma config to restore `--single-run=false`
  until karma-runner/karma-sauce-launcher#217 is fixed.
@drzraf drzraf mentioned this pull request Sep 16, 2020
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.

None yet

2 participants