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

Add e2e tests for rollup #681

Merged
merged 1 commit into from Jan 13, 2020
Merged

Add e2e tests for rollup #681

merged 1 commit into from Jan 13, 2020

Conversation

kevin940726
Copy link
Contributor

What's the problem this PR addresses?

Add e2e tests for rollup (#368)

  • Tree-shaking
  • Multiple entry modules
  • With NPM packages
  • Peer dependencies
  • Babel
  • rollup-plugin-postcss

Please tell me what other plugins/examples should I add :)

echo ".app { color: red; }" | tee src/style.css

yarn rollup -c
[[ "$(cat dist/bundle.css)" = ".style_app__3FC6W { color: red; }" ]]
Copy link
Member

Choose a reason for hiding this comment

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

Is the 3FC6W stable? Can it cause the build to periodically fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's a hash of the css file, which means that if we don't change the content of the css file, and the hash function never changed, it would be fine.

However, rollup also do not guarantee that the hashing function would never changed, we could argue that the result is not stable... Maybe we should think of other alternatives, any idea?

Copy link
Member

Choose a reason for hiding this comment

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

Typically this can be solved by doing a replace operation: | sed 's/__[0-9A-F]+/__xxx/'. This way the result is always stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a regex approach with =~ would be much cleaner 🤔? I can open up a follow up PR if that sounds good to you 😉

@arcanis
Copy link
Member

arcanis commented Jan 13, 2020

That's great! It sounds like the PnP plugin for Rollup isn't even needed anymore as long as one use the CJS plugin? Awesome!

Please tell me what other plugins/examples should I add :)

I think Gulp and Nuxt.js would be valuable. Parcel 2 as well, although I'm not 100% sure everything is ready on their side (cc @mischnic / parcel-bundler/parcel#3582?)

@arcanis arcanis merged commit df7c5bf into yarnpkg:master Jan 13, 2020
@kevin940726 kevin940726 deleted the rollup-e2e branch January 13, 2020 08:32
@kevin940726
Copy link
Contributor Author

Off-topic, but I think our gatsby e2e tests is flawed, I don't think that gatsby work in yarn berry now 🤔, I can open up another issue to track it (or just leave a comment in #368?)

@mischnic
Copy link
Contributor

👋 The only blocker at the moment is parcel-bundler/parcel#3210 (plugins aren't resolved relative to the config that specifies them - a shortcut was taken that is now broken because PnP enforces declared deps).

@arcanis
Copy link
Member

arcanis commented Jan 13, 2020

Off-topic, but I think our gatsby e2e tests is flawed, I don't think that gatsby work in yarn berry now 🤔, I can open up another issue to track it (or just leave a comment in #368?)

Sure, open an issue! Note that Gatsby at least used to work, since our website actually uses it (in packages/gatsby) 😃

The only blocker at the moment is parcel-bundler/parcel#3210 (plugins aren't resolved relative to the config that specifies them - a shortcut was taken that is now broken because PnP enforces declared deps).

Nice! Is it already possible for us to write an E2E that only makes use of parcelrcs without extend?

@mischnic
Copy link
Contributor

Nice! Is it already possible for us to write an E2E that only makes use of parcelrcs without extend?

If there were a published version with that PR merged, yes. 😬

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

3 participants