Skip to content

Refactor | Move the code into different functions - #291

Merged
bors[bot] merged 12 commits into
developit:masterfrom
MatiasOlivera:feature/index-refactoring
Jan 10, 2019
Merged

Refactor | Move the code into different functions#291
bors[bot] merged 12 commits into
developit:masterfrom
MatiasOlivera:feature/index-refactoring

Conversation

@MatiasOlivera

Copy link
Copy Markdown
Contributor

Issue #289

@paulmelnikowpaulmelnikow left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I read through these changes – they look good to me!

There's more that can be improved here, I think, though refactorings are sometimes easier to read if code is moved "as is" (as was done here) and then improved later.

Tests are failing; haven't dug through to see if it's related to this change.

@MatiasOlivera

MatiasOlivera commented Jan 2, 2019

Copy link
Copy Markdown
ContributorAuthor

@paulmelnikow Yes, there are other things that can be improved.

@ForsakenHarmony

Copy link
Copy Markdown
Collaborator

yeah, tests are unrelated I think, just stupidity on uglify's part

@ForsakenHarmony

Copy link
Copy Markdown
Collaborator

Feel free to update the snapshots

* Before pkgName was equal to undefined
* Allow passing no-pkg and no-pkg-name tests
@MatiasOlivera

MatiasOlivera commented Jan 4, 2019

Copy link
Copy Markdown
ContributorAuthor

I can't update the snapshots, because the tests are working on my machine 😱 Probably its something related to the dependencies.

Update: And now are broken, I don't know why.

@Andarist

Copy link
Copy Markdown
Collaborator

Maybe you have to clean npm cache and try to reinstall? If that won't help I'll look into it.

@ForsakenHarmony

Copy link
Copy Markdown
Collaborator

remove node_modules and package-lock.json and or try yarn

@MatiasOlivera

MatiasOlivera commented Jan 4, 2019

Copy link
Copy Markdown
ContributorAuthor

I did what you said. For some reason, the tests are creating new folders with the TS declaration files (index.d.ts). These files already exist in each /dist folder. And the snapshots are failing because it is comparing the wrong directories

@ForsakenHarmony

Copy link
Copy Markdown
Collaborator

that's very weird

@ForsakenHarmony

Copy link
Copy Markdown
Collaborator

bors r+

borsBot added a commit that referenced this pull request Jan 10, 2019
291: Refactor | Move the code into different functions r=ForsakenHarmony a=MatiasOlivera
Issue #289 Co-authored-by: Matías Olivera <matiasolivera@outlook.com>
@bors

borsBot commented Jan 10, 2019

Copy link
Copy Markdown
Contributor

Build succeeded

@bors
borsBot merged commit 1347042 into developit:masterJan 10, 2019
Comment threadsrc/index.js
entries: options.entries,
cwd,
source: options.pkg.source,
module: options.pkg.module,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I’m curious to learn, unless this is a mistake; how is module used as an input here?

Sign up for freeto 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.

6 participants

@MatiasOlivera@ForsakenHarmony@Andarist@jonathantneal@paulmelnikow@developit