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

Make deferred init works for rollup #344

Merged
merged 2 commits into from
Jan 7, 2022
Merged

Make deferred init works for rollup #344

merged 2 commits into from
Jan 7, 2022

Conversation

megabuz
Copy link
Contributor

@megabuz megabuz commented Jan 3, 2022

I've tried to use ohm-js with rollup and I've faced the issue of circular dependency in pexprs.js.

Based on specification all imports should be evaluated prior to the body of module doing the importing, so in case when pexprs requires bunch of extensions, e.g. pexprs-allowsSkippingPrecedingSpace, rollup includes these extensions in the final bundle before pexprs. However they depend on pexprs and I'm receiving error similar to #185 (comment)

In this PR I moved body of pexprs to separate file. It should make rollup module ordering work correctly.

@megabuz
Copy link
Contributor Author

megabuz commented Jan 4, 2022

I found several similar problems, I'm trying to fix them

@megabuz megabuz changed the title Move body of pexprs.js to separate file Make deferred init works for rollup Jan 4, 2022
@megabuz
Copy link
Contributor Author

megabuz commented Jan 4, 2022

So upon further testing I found that any require should be able to execute correctly before module that calls it.
I moved some deferred inits from main.js and Semantics.js to separate files to break this ordering dependency

@pdubroy
Copy link
Contributor

pdubroy commented Jan 6, 2022

@megabuz Thanks for taking this on! Is this PR ready for review? It seems to contain some unrelated changes. Can you trying rebasing your branch onto master?

@megabuz
Copy link
Contributor Author

megabuz commented Jan 6, 2022

@pdubroy it is ready to review.
Could you point out unrelated changes? All changes should be related to breaking circular dependencies

@pdubroy
Copy link
Contributor

pdubroy commented Jan 7, 2022

@megabuz Great! And sorry — my mistake, at a quick glance I thought some of these changes were related to something else.

Thanks again, and I'll try to review this in the next day or two.

@pdubroy pdubroy merged commit a78fc60 into ohmjs:master Jan 7, 2022
@pdubroy
Copy link
Contributor

pdubroy commented Jan 7, 2022

Awesome, thanks for doing this! It looked good to me so I've gone ahead and merged it.

@pdubroy
Copy link
Contributor

pdubroy commented Jan 7, 2022

Just published v16.1.1 which contains these changes.

pdubroy added a commit that referenced this pull request Jan 8, 2022
Fixes #185. Now that Ohm can be built with Rollup (see #344), we can ship a proper ES module via a Rollup build step.
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