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

Attempt to Fix #299 #313

Closed
wants to merge 1 commit into from
Closed

Attempt to Fix #299 #313

wants to merge 1 commit into from

Conversation

frytyler
Copy link

@frytyler frytyler commented Oct 5, 2018

Hi,

I saw #299 and tried to get it working however it turned out to be a fairly deep rabbit role. I seem to be stuck now anyone have any advice?

@ghost
Copy link

ghost commented Oct 5, 2018

There were the following issues with this Pull Request

  • Commit: c6138d3
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@SachsKaylee
Copy link
Collaborator

SachsKaylee commented Oct 7, 2018

Hello @frytyler!
Thank you very much for tackling this!

This is some really strange ES6/CJS module interop going on here.
The build currently fails with this message: Error: 'default' is not exported by node_modules\stringify-object\index.js

If one looks at the source code of this module, it turns out if does indeed not have a ES6 style default export, but instead exports its function directly.

module.exports = (val, opts, pad) => {
	const seen = [];
// ...

This is all fine in CJS land, but we are running into a problem since import stringify from 'stringify-object' essentially translates to const stringify = require('stringify-object').default.

Normally this sort of module system interop should be handled by the bundler for us. (And it is correctly handled e.g. for the is-plain-object dependency, which is also using the same module system.)

Maybe @danoc can help out with this, since there is some special handling for stringify-object in rollup.config.js#L31 by him?


Also, please make sure your commits comply with the requirements for this project. This configuration is used to lint the commits: https://www.npmjs.com/package/@commitlint/config-angular

https://superuser.com/questions/751699/is-there-a-way-to-edit-a-commit-message-in-github/751909

@frytyler
Copy link
Author

frytyler commented Oct 7, 2018

@PatrickSachs Hey ya it was very strange. I basically had to covert the whole project to support Babel 7 then I ran into this issue.

So I guess that has to also be considered before to much time is spent trying to fix this issue. Does your team want to transition to Babel 7 now or wait till later.

@SachsKaylee
Copy link
Collaborator

I personally see no issue with transitioning to Babel 7 as long as we don't cause any problems for the end users in the process.
However, I am not sure if I am authorized to make a decision on that. An actual member of the Algolia organization like @vvo might be able to give some insight on who gets to make these decisions.

@Haroenv
Copy link

Haroenv commented Oct 9, 2018

Since this is used as a dev dependency for users, I don't see why transitioning to babel 7 here would have an impact on users. It could be a major or breaking change if you want to be extra careful?

@vvo
Copy link
Contributor

vvo commented Oct 9, 2018

As long as we compile down to the same platforms than today there's nothing wrong in upgrading all dependencies. Find out which platforms we supported before (most probably any platform supporting es2015) and compile down to the same platforms and you are good to go.

If later on you want to compile to only more modern platforms, that's also a possibility but the easiest is usually to do it in different steps and only when needed (for example for compile size issues or compatibility issues we might not have here).

@vvo
Copy link
Contributor

vvo commented Oct 9, 2018

@PatrickSachs I trust your judgment here it's the right mindset (think about users first)

@vvo
Copy link
Contributor

vvo commented Oct 9, 2018

thanks @Haroenv for jumping here too

@armandabric
Copy link
Collaborator

I'm trying to make the upgrade on rollup (see #341) and I stuck with the issue as you in this PR. I will try to find a solution on this

@armandabric
Copy link
Collaborator

armandabric commented Jan 27, 2019

Some new digging into the issue make found a workaround. It will be shipped by the PR #351

@frytyler Thanks for your help with this 👍

@frytyler
Copy link
Author

Can this be closed? I don't have time to continue the work here

@armandabric armandabric closed this Jun 9, 2019
@armandabric
Copy link
Collaborator

Thanks for your try!

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

5 participants