regenerator icon indicating copy to clipboard operation
regenerator copied to clipboard

Refactor/es modules

Open Andarist opened this issue 6 years ago • 7 comments

This fixes #302

@benjamn So Im mostly done when it comes to refactoring regenerator-runtime.

The missing piece is to change regenerator-transform so it adds an import statement to the regenerator-runtime module. Im not strong with AST (although I know the basics) so an advice how this should be handled would be greatly appreciated! :)

Whats the best way to add a single import statement for a file with multiple generators? How we differentiate what type of import (es/cjs) should be added? Do we need to support a use case of including the regenerator itself in the outputted bundle? When?

Andarist avatar Jul 23 '17 19:07 Andarist

Sorry this I couldn't get the PR to show up quite the way I wanted. The intent is to reorganizing the tests so that all the test code isn't written four times. You can see how that works by looking at this file: https://github.com/plangrid/prop-types/blob/environment-testers/tests/arrayOf-test.js

The tests are written in a callback which is then run once for each environment. The results look like this: Screen Shot 2019-08-30 at 3 53 21 PM

Note that one outstanding issue is that the describe text for each test isn't completely accurate, but that is already the case with the copy/paste strategy.

conartist6 avatar Aug 30 '19 22:08 conartist6

Primarily I wish to know if a change like this would be merged. If it would not be I don't want to spend my time moving the rest of the code around.

conartist6 avatar Aug 30 '19 22:08 conartist6

I'm not saying there's no cost, but I'm pretty sure the benefit is worth it. Though it is difficult to tell, each of those existing test files tests arbitrary and subtly different things. It's 4x harder to write meaningful test code for any method, since most dev time is spent copying and pasting and working with the subtle differences of what goes where. Do I put the exact same code in each of the 4 files, just with different helpers? Many existing validator tests seem to feel that it isn't as important to test certain types of functionality in certain configurations. As a user of the package I just want to know that all the possible usages work as expected for each possible import style. Why would it not be valuable for the test code to be structured to match that expectation?

conartist6 avatar Aug 31 '19 20:08 conartist6

@ljharb Could you help me any further? What would I have to do to convince you? If you can't be convinced, can you resolve for me whether the intention is to test all use cases in all environments, and if it isn't, I would like to know what the criteria for a usage being tested in a particular environment is.

conartist6 avatar Sep 03 '19 20:09 conartist6

@conartist6 since you made this PR from a fork that's not your own, i don't think i'll be able to rebase this for you; can you rebase this?

ljharb avatar Nov 29 '21 07:11 ljharb