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

V6 #216

Merged
merged 65 commits into from
Nov 2, 2019
Merged

V6 #216

merged 65 commits into from
Nov 2, 2019

Conversation

davidtheclark
Copy link
Collaborator

@davidtheclark davidtheclark commented Oct 6, 2019

Development of V6 has taken longer than expected, so I've opened the branch v6 for that work and reverted master to 5.2.1 to avoid confusion.

We'll merge v6 work into this branch, then merge it into master when it's ready for release.

koenpunt and others added 30 commits May 18, 2019 08:35
The "coverage" script looks to have been replaced previously using jest's configuration. I checked both CI scripts and do not see this being used anywhere
… excess eslint config file

note: convert eslint config to js - enables proper comments and variables
…iple Cache types and standardize cacheWrapper calls
@chrisblossom
Copy link
Contributor

I think I solved all open issues including removing the hacky code from index.test.ts. I'm okay with merging and releasing at this PR's current state as long as everyone else is.

@olsonpm can you confirm this is what you had in mind: 60337c9

Questionable commits:

Object.freeze(defaultLoaders): 7a0f0e6

I am not sure if Object.freeze this will slowdown the initial load time of this library. I doubt it will considering how small of an object it is.

Remove yarn.lock fad0ebc

This closes #206. If this is not wanted, please revert the commit and close the issue. It isn't a big deal to me either way. I added it because it was the last thing I could think of that I might have put a PR in for v6.

@olsonpm
Copy link
Contributor

olsonpm commented Oct 23, 2019

i'm not at my computer but the only thing i would add is the file name in the error message similar to how the error is caught and the filename prepended to error message in the loader above

@chrisblossom
Copy link
Contributor

i'm not at my computer but the only thing i would add is the file name in the error message similar to how the error is caught and the filename prepended to error message in the loader above

Good idea. I'll add that tomorrow and hopefully figure out why the windows tests are failing.

@chrisblossom
Copy link
Contributor

chrisblossom commented Oct 23, 2019

In 358104c I added the file path to the yaml error and fixed the windows errors in db8f2cd.

Quick note about the removing yarn.lock commit. I'm not trying to force the change by committing it, it really doesn't matter to me. I just wanted to move on from this PR and that was the last thing I could think of. Please revert the commit for any reason.

Are there any other changes anyone thinks should be done?

Comment on lines 523 to 525
// This test does not work with lazy loading JS -- TODO: fix or remove
// eslint-disable-next-line jest/no-disabled-tests
describe.skip('ensure import-fresh is called when loading a js file', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure how to fix this test. I don't understand the point of the test, don't we know that by requiring it?. I personally think we should just remove it and move on.

@chrisblossom
Copy link
Contributor

chrisblossom commented Oct 23, 2019

Commit 08ebcd5 I took the idea from #214 and lazy loaded all default loaders. Any negatives from doing this?

@davidtheclark
Copy link
Collaborator Author

@chrisblossom Great work.

  • Object.freeze seems fine to me.
  • The lazy loading seems like a good idea and I don't know any reasons not to. We'll see if anybody finds any!
  • I don't know if I'm fully on board about yarn.lock but I or others can re-add it later if we think it's important, no big deal.
  • Deleting that import-fresh test seems ok to me. I will say after this experience I'm kind of skeptical about whether it's worth type-checking test files, because it seems to make stubbing/mocking/spying so much harder, with unclear benefits. But we have it working for now!

I pushed commit ed83eaa with some documentation and changelog adjustments. I think we're ready to launch.

Can anybody please chime in if you think we need to wait? If not, I'll merge & publish the release tomorrow (Sunday).

@chrisblossom
Copy link
Contributor

I can't think of anymore changes and I'm good with merging / releasing.

Deleting that import-fresh test seems ok to me. I will say after this experience I'm kind of skeptical about whether it's worth type-checking test files, because it seems to make stubbing/mocking/spying so much harder, with unclear benefits. But we have it working for now!

The import-fresh issue had nothing to do with types. It is a jest mocking / require issue.

@davidtheclark
Copy link
Collaborator Author

I released V6 this morning — decided not to spent more time fussing about the lazy requiring, and just see if users have problems with it (the stakes, I think, are pretty low). Nice work! 🍰 🍰 🍰

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

6 participants