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

support umd #333

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

support umd #333

wants to merge 3 commits into from

Conversation

graingert
Copy link

No description provided.

src/ng-flow.js Outdated

return 'flow';
}));
Copy link
Author

@graingert graingert Jul 5, 2017

Choose a reason for hiding this comment

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

300 lines is not enough to warrant splitting this module into multiple files.

/* for module loading using webpack or similar package bundlers */
window.Flow = require('./dist/ng-flow-standalone');
module.exports = 'flow';
/* backwards compatable with deprecated index.js*/
Copy link
Author

Choose a reason for hiding this comment

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

this file is ignored by commonjs loaders because of the 'main' key in package.json so I log a warning.

@@ -1,3 +1,7 @@
/* for module loading using webpack or similar package bundlers */
window.Flow = require('./dist/ng-flow-standalone');
Copy link
Author

Choose a reason for hiding this comment

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

mutating window in a commonjs module is not acceptable.

@AidasK
Copy link
Member

AidasK commented Jul 6, 2017

@evilaliv3 Can you review this? I haven't used yarn or webpack so I can't test this. But as long as all tests are passing, it is good for me.

I think this will be a breaking change for all of our users. We can release it under a major version.

@graingert
Copy link
Author

@AidasK ok I'll change it so it's not breaking

}

if (typeof window === 'object') {
window.Flow = require('@flowjs/flow.js');
Copy link
Author

Choose a reason for hiding this comment

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

mutating window in a commonjs module is not acceptable, but I do it here for require('@flowjs/ng-flow/index') for backwards compatibility reasons.

@AidasK
Copy link
Member

AidasK commented Jul 6, 2017

You have also removed ng-flow.js from dist folder, this is bc.

I am not against of breaking things. There is nothing wrong in releasing a major version if it is needed.

@graingert
Copy link
Author

You have also removed ng-flow.js from dist folder, this is bc.

oh woops, that's an error.

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.

2 participants