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

refactor: allow ES import for cloud string if package type is module #7560

Merged
merged 10 commits into from
Sep 14, 2021

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 9, 2021

New Pull Request Checklist

Issue Description

Currently, if package type is module, specifying a cloud string does not work.

Related issue: #7559

Approach

Checks env variable npm_package_type, which is autoset, and if it's a module, uses import() instead of require()

TODOs before merging

  • Add test cases
  • Add entry to changelog

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 9, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #7560 (7af6c16) into master (fdb7dfb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7560   +/-   ##
=======================================
  Coverage   93.94%   93.95%           
=======================================
  Files         181      181           
  Lines       13279    13281    +2     
=======================================
+ Hits        12475    12478    +3     
+ Misses        804      803    -1     
Impacted Files Coverage Δ
src/ParseServer.js 90.00% <100.00%> (+0.11%) ⬆️
src/ParseServerRESTController.js 98.50% <0.00%> (+1.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdb7dfb...7af6c16. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Sep 10, 2021

Could you resolve this conflict? Sorry, this will go away with release automation, hopefully by this month.

@mtrezza mtrezza changed the title Allow ES import for cloud string if package type is module refactor: allow ES import for cloud string if package type is module Sep 14, 2021
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtrezza mtrezza merged commit 0225340 into parse-community:master Sep 14, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@Moumouls
Copy link
Member

Moumouls commented Nov 5, 2021

So as discussed with @mtrezza , here @dblythy we have an issue, import is async. This is a race issue. A developer do not have any guarantee that the code will be loaded before he try to run some cloud code functions. We have some recurrent CI fails due to this feature (https://github.com/parse-community/parse-server/actions/runs/1426220570)

Last months and also with #7525 we have evidence that the Parse server start up should be refactored with a clean async start function. Then we can drop the old constructor pattern (not flexible at all since we cannot return a promise)

For now the PR could introduce huge problem in a prod environment. i'll recommend to revert this PR.

I'll open an additional Issue with a first suggestion of the spec for the Parse Server start async fn. And also a suggestion of feature implementation to avoid a breaking change with a support of both constructor system and async fn during one major version.

mtrezza added a commit to mtrezza/parse-server that referenced this pull request Nov 10, 2021
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants