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

yaml plugin: provide file id to transform function #615

Merged
merged 5 commits into from
Oct 22, 2020
Merged

yaml plugin: provide file id to transform function #615

merged 5 commits into from
Oct 22, 2020

Conversation

janosh
Copy link
Contributor

@janosh janosh commented Oct 21, 2020

Rollup Plugin Name: YAML

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

Non-breaking change since people don't have to use the extra transformer argument.

Description

Reason for this change: I need to apply different content transformations depending on which file the data came from. Without knowing the file, the transformer function has to rely on brittle heuristics to identify the file based on its content (which can obviously change over time).

I think a new test case isn't needed for this but happy to provide one if someone thinks otherwise.

reason for this change: i need to apply different content transformations depending on which file the data came from

without knowing the file, the transformer function has to rely on brittle heuristics to identify the file based on the content loaded from it (which can obviously change over time)

non-breaking change since people don't have to use the extra transformer argument
Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

We still need a test case to make sure the correct path is being sent through. You can modify some of the existing tests to add that check.

@janosh
Copy link
Contributor Author

janosh commented Oct 21, 2020

@NotWoods Odd. If I run pnpm run test --filter ./packages/yaml locally, the tests are passing. Am I missing something?

@janosh
Copy link
Contributor Author

janosh commented Oct 22, 2020

Ah, different directory separators on UNIX ('/') and Windows ('\') is why it was failing.

Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

Thanks!

@NotWoods NotWoods merged commit 93eae80 into rollup:master Oct 22, 2020
@shellscape
Copy link
Collaborator

shellscape commented Oct 22, 2020

@NotWoods you didn't use conventional commit on that merge. that'll hose our versioning and changelog generation. can you amend that commit and force push?

I was able to get it.

@janosh
Copy link
Contributor Author

janosh commented Oct 23, 2020

@NotWoods Can you say when this will be published?

@shellscape
Copy link
Collaborator

@janosh we typically make weekly publish rounds for plugins. we have a lot moving parts this week so don't be surprised if this takes until early next week to be seen on NPM. (We're not quite ready for automated publishing)

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